Add rubocop settings and run safe autocorrect #1395
Conversation
|
Was thinking we can split this PR up for 2 people to review? |
.rubocop.yml
Outdated
| Enabled: false | ||
| StyleGuide: https://relaxed.ruby.style/#lintassignmentincondition | ||
|
|
||
| Layout/LineLength: |
There was a problem hiding this comment.
Are we going to enable this sometime in the future? I saw that Layout/LineLength is defined on line 22 and it doesn't seem to be part of the relaxed style guide.
There was a problem hiding this comment.
I removed this line. We are setting the line length to be 100 at the beginning of rubocop file
I can review 1-28. Can you assign another person? |
|
File 1-28 LGTM! |
|
Hi @xinyis991105 could you use the check box on the review page to indicate which files you have looked at? |
Already did. |
|
@xinyis991105 I think the checkmarks aren't saved unless you submit the review |
Are they visible now? |
|
HAHA i don't think so. I guess we don't get to see other people's checkmarks :/ |
|
@michellexliu Please prioritize roster upload! I will review the rest of this PR! |
app/models/assessment_user_datum.rb
Outdated
| 0.0 | ||
| when EXCUSED | ||
| nil | ||
| end | ||
|
|
||
| # TODO: final_score = apply_tweak(final_score) if final_score |
There was a problem hiding this comment.
The step of applying a tweak is now in app/models/submission.rb. Maybe delete this comment to avoid future confusion?
There was a problem hiding this comment.
okay sounds good.
app/models/assessment_user_datum.rb
Outdated
| 0.0 | ||
| when EXCUSED | ||
| nil | ||
| end | ||
|
|
||
| # TODO: final_score = apply_tweak(final_score) if final_score |
app/models/watchlist_instance.rb
Outdated
| if pending_watchlist? | ||
| contacted_watchlist! | ||
|
|
||
| raise "Failed to update watchlist instance #{id} to contacted" if !save && !save |
There was a problem hiding this comment.
Now this code doesn't make sense 😂 . Could you remove the second !save?
app/models/watchlist_instance.rb
Outdated
| if not self.save | ||
| raise "Failed to update watchlist instance #{self.id} to resolved" unless self.save | ||
| end | ||
| raise "Failed to update watchlist instance #{id} to resolved" if !save && !save |
There was a problem hiding this comment.
Changes requested. See comments.
Tested behaviorally as well. Found a bug #1401 that is replicated on master so it's not linting specific. Otherwise, everything functions normally. Thanks a lot!
|
Thanks for the review! @xinyis991105 |
| @@ -16,7 +19,7 @@ AllCops: | |||
| Rails: | |||
| Enabled: true | |||
|
|
|||
| Metrics/LineLength: | |||
| Layout/LineLength: | |||
There was a problem hiding this comment.
Here is the linelength stuff
There was a problem hiding this comment.
Just to make sure, does removing "Enabled: false" introduce further changes if linting is run again? I'm not sure whether all lines are below 100 right now.
There was a problem hiding this comment.
It will probably introduce more things to be manually fixed, but not things that can be automatically fixed. I ran it again and there was nothing else that can be safely autofixed
There was a problem hiding this comment.
Ok then let's not run it again for now.
Description
Add Styles from https://relaxed.ruby.style/ into our rubocop settings.
Ran safe autocorrect.
Motivation and Context
This is part 1 of our efforts to organize our ruby codebase better.
How Has This Been Tested?
Ran server
Types of changes
Checklist:
Other issues / help required
If unsure, feel free to submit first and we'll help you along.