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

style: nph formatting #1067

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

style: nph formatting #1067

wants to merge 2 commits into from

Conversation

AuHau
Copy link
Member

@AuHau AuHau commented Jan 14, 2025

This PR implements code formatting using nph of folders codex/ and tests/.

It is enforced on CI level using https://github.com/arnetheduck/nph-action. Nice thing it can suggest formatting changes in case somebody forgets to format its PR before submitting it.

More details on how to use it can be found in README: https://github.com/codex-storage/nim-codex/pull/1067/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R60

This work is based on nwaku implementation of nph - so kudos to them!

@AuHau AuHau requested review from dryajov, gmega and emizzle January 14, 2025 14:06
@AuHau AuHau force-pushed the style/nph-implementation branch from db6bd63 to ab566e7 Compare January 14, 2025 14:13
Copy link
Contributor

@benbierens benbierens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have a precommit hook and a workflow checking step. Even if the style itself isn't perfect, I think this is still a very good thing to have.

@gmega
Copy link
Member

gmega commented Jan 14, 2025

Yes we know you just want to have your name amongst the contributors with the most lines added/deleted, @AuHau ...

Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a more serious note: I'm happy to have nph, but I think we need to make sure versions match locally and in CI.

Makefile Show resolved Hide resolved
@AuHau
Copy link
Member Author

AuHau commented Jan 15, 2025

@emizzle and @gmega - CI action pinned. Please re-review ;-)

@AuHau AuHau requested a review from gmega January 15, 2025 07:44
Copy link
Contributor

@emizzle emizzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arnetheduck
Copy link

committer

see https://github.com/arnetheduck/nph/blob/master/format-git-repo.sh#L12 for how to not become the target of future git blame - after all, the last to change the code is the one that is responsible for it's maintenance until the next sucker comes around ;)

@AuHau
Copy link
Member Author

AuHau commented Jan 15, 2025

committer

see arnetheduck/nph@master/format-git-repo.sh#L12 for how to not become the target of future git blame - after all, the last to change the code is the one that is responsible for it's maintenance until the next sucker comes around ;)

That is a super cool tip! Thanks will employ it ;-)

@AuHau AuHau force-pushed the style/nph-implementation branch from e0eceb4 to 1bf6dd1 Compare January 15, 2025 09:40
@arnetheduck
Copy link

something to be mindful of: when you merge this with the usual non-fast-forward squash merge, the commit hashes will change invalidating git-blame-ignore-revs - better merge manually or create ignore-revs separately

If you are setting up fresh setup, in order to get `nph` run `make build-nph`.
In order to format files run `make nph/<file/folder you want to format>`.
If you want you can install Git pre-commit hook using `make install-nph-commit`, which will format modified files prior commiting them.
If you are using VSCode and the NimLang extension you can enable "Format On Save" that will format the files using `nph`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this "Format On Save" or is it nim.lintOnSave? There is also editor.formatOnSave option, and perhaps it also has to be set to true, but formatOnSave is just a generic VSCode option if I remember correctly.

image

Also, you could consider adding a link to the NimLang to remove any doubts about the extension.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Yeah it is, it is nim.formatOnSave. Will clarify though - thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I did some tests. So it looks like nim.lintOnSave is actually depreciated and nim.formatOnSave does exist on NimLang extension. Setting either editor.formatOnSave or nim.formatOnSave does the job (one of them need to be set to true, the order does not seem to matter).

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.

6 participants