Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Code Climate issues #82

Open
aaron-junot opened this issue Dec 28, 2018 · 15 comments
Open

Fix Code Climate issues #82

aaron-junot opened this issue Dec 28, 2018 · 15 comments
Labels

Comments

@aaron-junot
Copy link
Member

We recently added in a Code Climate integration, and it complains about some issues. It's not a huge deal, but it would be nice to address these issues before they get more out of hand.

https://codeclimate.com/github/OperationCode/resources_api/issues

Ideally, I would like to see the migrations folder ignored by code climate. I only want our actual source code and tests to be inspected by this tool. Things that are build tools, build artifacts, etc should not be included in this scan.

@aaron-junot aaron-junot added the good first issue Good for newcomers label Dec 28, 2018
@Optimus-PrimaNocta
Copy link
Contributor

After looking at Code Climates documentation I created a .yml to tell Code Climate to ignore folders and files we specify. I added the migrations folder.

@aaron-junot
Copy link
Member Author

Merged #86

@apex-omontgomery
Copy link
Member

What else is needed for this?

@aaron-junot
Copy link
Member Author

There are now 17 issues. Follow the link to find out more.
Screen Shot 2019-04-18 at 10 24 50 PM

https://codeclimate.com/github/OperationCode/resources_api/issues

@Optimus-PrimaNocta
Copy link
Contributor

It doesn't look like any of those would break anything, just mostly complexity issues. Did we want to add the pages to the .yml to ignore them?

@aaron-junot
Copy link
Member Author

I think we can certainly raise the limit for LOC, I think 250 is a bit of a stretch. Alternatively, it might make sense to break up routes.py and test_routes.py into smaller files, but I don't think they're too big right now, personally.

As for the code complexity issues, if those functions can be refactored, that would be nice, but again, I'm not too worried about it. I'm not even sure how to fix the "too many early returns" thing. Maybe using variables and returning the value at the end? I think that a long && || string would be much harder to grok than just "return False"...

How about this: if you want to tweak the yaml file to get rid of all of them, we can review the rules in the PR and make specific decisions point by point. If we want to actually fix the code in any case, we'll do that.

@aaron-junot
Copy link
Member Author

@rooshs481 A lot of the code climate issues are with the tests. Please break this into 2 PRs, one where we just address the app changes and another where we just address the test changes. Right now, I trust the tests to tell us that everything we promise in the docs is working properly and I believe that we can use that confidence to make changes to the app only.

When we start messing with the tests, I don't want any app changes because I want to only have to focus on ensuring the tests still give us the same confidence. Thanks for taking this on.

@avats-dev
Copy link

@aaron-suarez can I work on this? Also, the link given above for code climate issues gives a 404.

@aaron-junot
Copy link
Member Author

@avats-dev I believe that was a 404 because we switched the default branch from master to main but never updated the default branch in Code Climate. Thanks for letting me know, I made the switch and it appears to be working now. Looks like there's 59 issues identified, so it appears you have your work cut out for you 😆 Thanks for picking this up

@avats-dev
Copy link

@aaron-suarez yeah, now link works and I checked out the errors and I'll get started on this. I'm a bit busy with some personal work but I want to contribute, so I might take some time to get this done. Hope that's okay. Thanks.

@aaron-junot
Copy link
Member Author

This has sat for a very long time, I would be shocked if someone else picked it up. So take your time my friend

@king-11
Copy link
Contributor

king-11 commented Oct 22, 2020

@aaron-suarez can I work on some of these issues

@aaron-junot
Copy link
Member Author

Sure @king-11! Maybe mention which ones you're doing so @avats-dev can make sure that he's working on different ones. There were over 50 issues last I checked so there should be plenty to go around

@king-11
Copy link
Contributor

king-11 commented Oct 23, 2020

@avats-dev I will be working on minor duplication Issues to make code DRY which I found after applying the filter do tell if you have worked on some

@danielburn5
Copy link

@avats-dev @king-11 Have you folks started work on any of these issues? I'd like to contribute but don't want to duplicate effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants