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.

Sign up for GitHub

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

Jump to bottom

add automation for consistent formatting and linting #260

Open
kbroch-rivosinc opened this issue Nov 13, 2024 · 3 comments · May be fixed by #266
Open

add automation for consistent formatting and linting #260

kbroch-rivosinc opened this issue Nov 13, 2024 · 3 comments · May be fixed by #266
Assignees
@kbroch-rivosinc
Labels
enhancement New feature or request

Comments

@kbroch-rivosinc
Copy link
Collaborator

Consistently formatted yaml, json, etc makes it easier for multiple developers and reduces git diffs.

Formatting and linting that can be run on at git commit time with a git hook, reduces the need for this later in PRs (Github action can be used to confirms that it was done)

There are tools to manage pre-commit hooks like:

Since this project is a multilingual project:

❯ tokei -C --sort lines
===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 HTML                  136        94823        55057            0        39766
 YAML                 1146        93645        81165         2961         9519
 Ruby                   34        16690        11701         2340         2649
 JSON                   12         5883         5855            0           28
 JavaScript              7         4158         4096            9           53
 Ruby HTML              20         3001         2668            0          333
 Rakefile               10         1914         1582           38          294
 AsciiDoc                7         1816         1397            0          419
 CSS                     3          556          512            7           37
 BASH                    9          219          165           16           38
 Module-Definition       1           37           25            0           12
 Markdown                1            1            0            1            0
===============================================================================
 Total                1386       222743       164223         5372        53148
===============================================================================

I would suggest using pre-commit to manage the hooks but I'm open to exploring other tooling. (I do realize that since pre-commit is python based I'd be adding another language to the dev dependencies)

I'm happy to put in a PR once the discussion done.
@kbroch-rivosinc kbroch-rivosinc added the enhancement New feature or request label Nov 13, 2024
@dhower-qc
Copy link
Collaborator

Love that idea. As mentioned in Issue #246, it might also be nice to have the keys sorted in YAML files to ensure scripted changes produce reasonable diffs (Since different YAML libraries will likely emit in different orders).

@kbroch-rivosinc
Copy link
Collaborator Author

kbroch-rivosinc commented Nov 13, 2024 •
edited
Loading

I did some exploration of yaml formatting with following tools:

Here's my findings:

  • yq:
    • pros:
      • does general formatting
      • sorting of keys yq --inplace '... | sort_keys(.)'
    • cons:
      • formatting replaces multi-line "description" values with quotes and "\n"
      • sorting can't be customized
  • prettier:
    • pros:
      • does general formatting (doesn't touch multiline description)
    • cons:
      • no sorting capabilities
  • yamlfmt: same as yq except doesn't sort at all

My recommendation based on this would be to move ahead with prettier formatting and writing a custom script to enforce keyword sorting with a specific ordering for some keyword if needed.

kbroch-rivosinc added a commit to kbroch-rivosinc/riscv-unified-db that referenced this issue Nov 13, 2024
@kbroch-rivosinc
initial pre-commit config
2eea098 
soft launch meaning only people with pre-commit tool installed would
see this run.  If you want to see full effect on files run:
`pre-commit run --all-files`

fixes riscv-software-src#260
kbroch-rivosinc added a commit to kbroch-rivosinc/riscv-unified-db that referenced this issue Nov 13, 2024
@kbroch-rivosinc
initial pre-commit config
8be77e2 
soft launch meaning only people with pre-commit tool installed would see this run.
(full launch would entail, running pre-commit on all files and  github action to enforce pre-commit)

If you want to see full effect on files run: `pre-commit run --all-files` I looked the results of this over and changes seem fine.

There's a lot of formatting changes to the html. (but some of the html looks minified so to be expected)
I'd suggest maintainers look over what the changes would be for all the files and the generate final docs.

fixes riscv-software-src#260
@kbroch-rivosinc kbroch-rivosinc linked a pull request Nov 13, 2024 that will close this issue
Open
kbroch-rivosinc added a commit to kbroch-rivosinc/riscv-unified-db that referenced this issue Nov 14, 2024
@kbroch-rivosinc
initial pre-commit config
38639c8 
soft launch meaning only people with pre-commit tool installed would see this run.
(full launch would entail, running pre-commit on all files and  github action to enforce pre-commit)

If you want to see full effect on files run: `pre-commit run --all-files` I looked the results of this over and changes seem fine.

There's a lot of formatting changes to the html. (but some of the html looks minified so to be expected)
I'd suggest maintainers look over what the changes would be for all the files and the generate final docs.

fixes riscv-software-src#260
@kbroch-rivosinc kbroch-rivosinc self-assigned this Nov 15, 2024
@kbroch-rivosinc
Copy link
Collaborator Author

Discussed this more with maintainers and going to reduce the number of hooks in this initial PR to just very simple:

  • end of line
  • end of file
  • symlink

The other hooks in this initial can then be put in later PRs.

kbroch-rivosinc added a commit that referenced this issue Nov 20, 2024
@kbroch-rivosinc
initial pre-commit config
b61d18b 
relates to  #260
kbroch-rivosinc added a commit that referenced this issue Nov 20, 2024
@kbroch-rivosinc
autofixes from pre-commit run --all-files based on hooks from last …
57a785a 
…commit

relates to #260
kbroch-rivosinc added a commit that referenced this issue Nov 20, 2024
@kbroch-rivosinc
add json related pre-commit hooks
9872c01 
relates to #260 and #267
@kbroch-rivosinc kbroch-rivosinc mentioned this issue Nov 20, 2024
Draft
kbroch-rivosinc added a commit that referenced this issue Nov 20, 2024
@kbroch-rivosinc
add yaml related pre-commit hooks
cd0ecb8 
relates to #260
@kbroch-rivosinc kbroch-rivosinc mentioned this issue Nov 20, 2024
Draft
kbroch-rivosinc added a commit that referenced this issue Nov 20, 2024
@kbroch-rivosinc
autofix from pre-commit run --all-files prettier
c8bacaa 
relates to #260

NOTE: has json changes from #295 as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kbroch-rivosinc kbroch-rivosinc

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

initial pre-commit config kbroch-rivosinc/riscv-unified-db
2 participants
@kbroch-rivosinc @dhower-qc