clerk-sdk-ruby icon indicating copy to clipboard operation
clerk-sdk-ruby copied to clipboard

chore: Add code formatting

Open dimkl opened this issue 1 year ago • 6 comments

dimkl avatar Feb 26 '24 12:02 dimkl

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?

dimkl avatar Feb 26 '24 13:02 dimkl

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?

gkats avatar Feb 26 '24 13:02 gkats

@gkats I found the following resources for rubocop linter:

  • github action: added step in main.yml as part of this PR
  • plugin for vim: https://github.com/ngmy/vim-rubocop
  • plugin vscode: https://github.com/rubocop/vscode-rubocop
  • i have also added running as a pre-commit hook with the following code:
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

dimkl avatar Apr 03 '24 23:04 dimkl

:clap: :clap: 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?

gkats avatar Apr 04 '24 10:04 gkats

  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

dimkl avatar Apr 04 '24 11:04 dimkl

git commit --no-verify

That works for me! :smile: :tada:

gkats avatar Apr 04 '24 11:04 gkats