resources_api
resources_api copied to clipboard
OperationCode
Fix Code Climate issues
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.
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.
There are now 17 issues. Follow the link to find out more.
https://codeclimate.com/github/OperationCode/resources_api/issues
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?
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.
@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.
@aaron-suarez can I work on this? Also, the link given above for code climate issues gives a 404.
@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
@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.
This has sat for a very long time, I would be shocked if someone else picked it up. So take your time my friend
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
@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
@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.
