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

maint: Add prettier pre-commit hook #3663

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

Conversation

mathbunnyru
Copy link
Contributor

prettier is a much better tool than using hooks from https://github.com/pre-commit/pre-commit-hooks - the style is much better, always consistent and it actually works

Comment on lines 18 to +19
hooks:
- id: rst-backticks
- id: rst-directive-colons
- id: rst-inline-touching-normal
- id: rst-backticks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This an example when existing hooks allow different styles (even in the same file)

@@ -5,28 +5,25 @@ repos:
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: fix-encoding-pragma
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is deprecated

Comment on lines -10 to -16
- id: check-yaml
exclude: ^.+(/tests/|/recipe/).+$
- id: check-toml
- id: check-json
- id: check-merge-conflict
- id: pretty-format-json
args: [--autofix]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These checks (except check-merge-conflict) no longer seem to be needed

- id: debug-statements
language_version: python3
# Autoformat: YAML, JSON, Markdown, etc.
- repo: https://github.com/rbubley/mirrors-prettier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a fork, since official prettier pre-commit hook has been archived:
http://github.com/prettier/prettier/issues/15742

Copy link
Member

Choose a reason for hiding this comment

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

Hum, I'm kind of reluctant here. As I'm in favor of general improvements, using something that just has been archived (even if a mirror is available) doesn't seem a good idea to me.
https://github.com/rbubley/mirrors-prettier seems to rather be a workaround to let people keep using the hook and there are no guarantees that it will be maintained in the long term.
I would highly suggest to wait until a stable solution is available (could be something used by a significant number of people, regularly maintained, or officially superseding the archived one in pre-commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an important gotcha: the https://github.com/rbubley/mirrors-prettier is not a mirror/fork of prettier ore pre-commit.
It's a small repo to make vanilla prettier work as pre-commit hook.
If you take a look at the source code, you'll see that the part which matters is less than 100 lines.
So, there is not much to maintain and the code to automatically update the version of the prettier is already there.

Even in the worst case scenario, this fork will stop updating, we will have some prettier version in this repo.
It's already working great for lots of files like json/yaml/toml/markdown.
I don't think these files have that much new syntax features added to them (unlike python/c++ code, where you really want tools to work with newer code as well).
So, it's unlikely we'll encounter issues due to not using the latest version.

And the sooner we add prettier, the less diff this PR will generate.

@@ -39,8 +39,7 @@ def template_substitute(contents):
today = datetime.date.today()
fmt_today = today.strftime("%B %d, %Y")

header_line = f"{name} {version} ({fmt_today})"
res += f"{header_line}\n{'=' * len(header_line)}\n\n"
res += f"# {name} {version} ({fmt_today})\n\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

releaser.py and changelog.py are changed manually by me, to make sure new changelogs are written correctly

@@ -40,15 +39,14 @@ Bug fixes:
- [libmamba, micromamba] Create empty base prefix with `env update` by @Hind-M in https://github.com/mamba-org/mamba/pull/3519
- [libmamba, micromamba] fix: Use POSIX-compliant scripts by @jjerphan in https://github.com/mamba-org/mamba/pull/3522
- [libmamba, micromamba] maint: Clarify `env` subcommand documentation in help menu (cont'd) by @jjerphan in https://github.com/mamba-org/mamba/pull/3539
- [libmamba] fix: Handle space in `mamba` and `micromamba` executable absolute paths by @NewUserHa in https://github.com/mamba-org/mamba/pull/3525
- [libmamba] fix: Handle space in `mamba` and `micromamba` executable absolute paths by @NewUserHa in https://github.com/mamba-org/mamba/pull/3525
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes prettier fixes double space and some other minor things in markdown text, don't think this is gonna cause trouble

@mathbunnyru
Copy link
Contributor Author

Please, let me know if you don't like PRs with improvements to tooling around the code quality.
I think this can greatly improve readability and consistency and we only need to run the tool once, and then all the fixes will be automatic.

@mathbunnyru mathbunnyru changed the title Add pretter pre-commit hook maint: Add pretter pre-commit hook Dec 7, 2024
@jjerphan jjerphan changed the title maint: Add pretter pre-commit hook maint: Add prettier pre-commit hook Dec 9, 2024
@jjerphan jjerphan added the release::ci_docs For PRs related to CI or documentation label Dec 10, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for this proposal, @mathbunnyru.

I am not against it, but I think we should ignore the formatting done by this PR then.

I am waiting for someone else from the team to comment.

@mathbunnyru
Copy link
Contributor Author

@jjerphan @Hind-M should I close this PR then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::ci_docs For PRs related to CI or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants