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

chore: Add code formatting #49

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

chore: Add code formatting #49

wants to merge 7 commits into from

Conversation

dimkl
Copy link
Member

@dimkl dimkl commented Feb 26, 2024

No description provided.

@dimkl dimkl requested review from agis and gkats February 26, 2024 12:18
@dimkl dimkl self-assigned this Feb 26, 2024
Copy link
Member

@gkats gkats left a 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?

Gemfile.lock Outdated Show resolved Hide resolved
@dimkl
Copy link
Member Author

dimkl commented Feb 26, 2024

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?

@gkats
Copy link
Member

gkats commented Feb 26, 2024

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?

@dimkl dimkl force-pushed the add-code-formatting branch 2 times, most recently from aef898d to c045cf7 Compare April 3, 2024 23:38
@dimkl
Copy link
Member Author

dimkl commented Apr 3, 2024

@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

@gkats
Copy link
Member

gkats commented Apr 4, 2024

👏 👏 Nice @dimkl!!

One thing I would suggest is to remove the pre-commit hook and replace it with:

  1. a script that runs the rubocop checks so that we can run it manually and
  2. a CI step which errors out if rubocop checks are failing (I guess this is done)

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?

@dimkl
Copy link
Member Author

dimkl commented Apr 4, 2024

  1. a script that runs the rubocop checks so that we can run it manually and

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 git commit --no-verify .... to bypass the hook.

2.a CI step which errors out if rubocop checks are failing (I guess this is done)

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.
I have added it as a step in the main GH action instead of a different check.
cc: @gkats

@gkats
Copy link
Member

gkats commented Apr 4, 2024

git commit --no-verify

That works for me! 😄 🎉

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.

None yet

2 participants