Skip to content

refactoring/codeclimate#249

Merged
Brantron merged 5 commits intomasterfrom
brantron/refactor
Sep 20, 2018
Merged

refactoring/codeclimate#249
Brantron merged 5 commits intomasterfrom
brantron/refactor

Conversation

@Brantron
Copy link
Copy Markdown
Contributor

will be pushing commits

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 11, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@65ae565). Click here to learn what that means.
The diff coverage is 97.61%.

@@            Coverage Diff            @@
##             master     #249   +/-   ##
=========================================
  Coverage          ?   80.24%           
=========================================
  Files             ?       57           
  Lines             ?     1083           
  Branches          ?      251           
=========================================
  Hits              ?      869           
  Misses            ?      155           
  Partials          ?       59

@Brantron Brantron force-pushed the brantron/refactor branch 5 times, most recently from 363bf13 to 1f3364f Compare September 11, 2018 13:49
@Brantron
Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Member

@dcocchia dcocchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you feel like this refactor based on the Code Climate report is worthwhile? I’d like to use it more if it is, but I also don’t want to force people to rearrange bits to make some tool happy if it doesn’t help :)

@Brantron
Copy link
Copy Markdown
Contributor Author

I would say it was 50/50. I feel like a tool like this is great as an overall guide but I don't think striving for all A's should be the goal (not that anyone stated it is the goal). There were some improvements I made that felt good but to get rid of all of the issues I definately felt I was refactoring for the tool vs maintainability and readibility. Code golf is fun but I felt like I was code golfing vs trying to improve a shared lib. I think I would prefer to go back and reintroduce some offenders before merging this.

@Brantron
Copy link
Copy Markdown
Contributor Author

Brantron commented Sep 11, 2018

I will push a few more commits later. They will reduce the file scores but I'm curious where the happy medium falls. Maybe a B or greater is a good goal. I think sensible rules around a good tool now are is better than waiting for the perfect tool and adopting none at all.

@dcocchia
Copy link
Copy Markdown
Member

Great feedback :) If you remove some of the code changes, let’s keep track of which rules seem arbitrary or too strict. We can change the code climate config to match our needs

@Brantron
Copy link
Copy Markdown
Contributor Author

Hmm, I reintroduced some of the code that was offending before and it's no longer an issue... I was definitely getting hits on too many return statements and too many args for a function. These are good rules overall and I think we would get value in keeping them.

@dcocchia
Copy link
Copy Markdown
Member

Interesting 🤔 Well, I think we can safely merge and keep track of our feelings on the code climate reports with future PRs to adjust how strict the rules are

@Brantron Brantron merged commit 21a8b87 into master Sep 20, 2018
@Brantron Brantron mentioned this pull request Sep 20, 2018
Brantron added a commit that referenced this pull request Sep 20, 2018
🏠 Internal
- `spacing`
	- #252 use longhand rules for spacing to avoid radium warnings (#252)

- `isEqual`
	- #249 refactor isEqual for better maintainability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants