-
Notifications
You must be signed in to change notification settings - Fork 10
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
chore: Add code formatting #49
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
How are we planning to use this, when is the formatter supposed to run?
I think we should at least run it manually until we create GH action to run make PRs fail if there are any lint changes found. What do you think? |
I think we should somehow enforce it from the start, so maybe consider adding a linter? Ideally, formatting should run automatically. Do you happen to know if there's any configuration that we can use for our editors that would perhaps format on every save? I've had success with "autoformat on save" using rufo in the past, perhaps it works similarly? |
566eff1
to
d2650c6
Compare
aef898d
to
c045cf7
Compare
c045cf7
to
ab697c8
Compare
@gkats I found the following resources for rubocop linter:
require 'english'
require 'rubocop'
ADDED_OR_MODIFIED = /A|AM|^M/.freeze
changed_files = `git status --porcelain`.split(/\n/).
select { |file_name_with_status|
file_name_with_status =~ ADDED_OR_MODIFIED
}.
map { |file_name_with_status|
file_name_with_status.split(' ')[1]
}.
select { |file_name|
File.extname(file_name) == '.rb'
}.join(' ')
system("rubocop #{changed_files}") unless changed_files.empty?
exit $CHILD_STATUS.exitstatus |
👏 👏 Nice @dimkl!! One thing I would suggest is to remove the pre-commit hook and replace it with:
The reason I'm suggesting we remove the pre-commit hook is that we should be allowed to commit changes without necessarily conforming to the linter in our local setup. Sometimes you have work in progress that you'd like to commit and the pre-commit hooks get in your way. The goal would be to ensure that the code that gets merged does not violate the linter rules. What do you think? |
I would suggest we keep the pre-commit hook as the norm would be to be compliant with our linter and adjust it to match the styles that the teams wants to use. If someone wants to by-pass the pre-commit hook they can use
I have already added it, that's why this PR fails. I have added some TODOs in the rubocop file to loosen up the lining rules for now but i will try to resolve them as part of this PR and remove the TODOs. |
That works for me! 😄 🎉 |
No description provided.