Skip to content

Prevent formatting symlink file - #1144

Merged
chuckwondo merged 6 commits into
earthaccess-dev:mainfrom
ana-sher:1143-pre-commit-hook-win-bug
Nov 17, 2025
Merged

Prevent formatting symlink file#1144
chuckwondo merged 6 commits into
earthaccess-dev:mainfrom
ana-sher:1143-pre-commit-hook-win-bug

Conversation

@ana-sher

@ana-sher ana-sher commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Prevent fixing symlink file
resolves #1143 LICENSE.txt fixing issue with end-of-file-fixer hook on Windows


📚 Documentation preview 📚: https://earthaccess--1144.org.readthedocs.build/en/1144/

@github-actions

github-actions Bot commented Nov 14, 2025

Copy link
Copy Markdown

Binder 👈 Launch a binder notebook on this branch for commit 1a7922d

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 906cf98

Binder 👈 Launch a binder notebook on this branch for commit 05fbfba

Binder 👈 Launch a binder notebook on this branch for commit b2f30e4

Binder 👈 Launch a binder notebook on this branch for commit 3783875

@ana-sher
ana-sher marked this pull request as ready for review November 14, 2025 21:11
Comment thread .pre-commit-config.yaml Outdated
Comment on lines +16 to +17
- id: check-added-large-files
# Check for common mistakes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ana-sher! It looks like you may have forgotten to skip the yamlfmt hook when you committed your change, but I think that's because you used VSCode to commit this change, rather than making the commit from the command line, where you would be able to specify SKIP.

Can you revert the changes in this file, other than the single line for excluding docs/LICENSE.txt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @chuckwondo, yes, that's exactly what happened 💯 Reverted and fixed by running from the command line.

@chuckwondo

Copy link
Copy Markdown
Contributor

@ana-sher, thanks again for your first contribution!

It looks like your branch is out of date with the main branch. Can you pull in the latest changes from main?

@ana-sher
ana-sher requested a review from chuckwondo November 14, 2025 22:00
@chuckwondo

Copy link
Copy Markdown
Contributor

Much better :-)

I think I found a solution to the yamlfmt issue, if you want to give it a try. It works for me, but that's because I'm running macOS, so if you want to try the following on your Windows machine, that would be great.

If it works, I'd like you to add it to this PR so that it is a complete fix for #1143.

Please do the following:

First, create a file named scripts/yamlfmt.py (the scripts directory already exists in the repo), with the following contents:

"""Helper script that automatically skips the yamlfmt pre-commit hook on Windows.

This is not intended to be called directly.  It is configured to be called by
pre-commit via the configuration in .pre-commit-config.yml for the yamlfmt hook.

Due to https://github.com/google/yamlfmt/issues/263, comments in yaml files are
not handled properly under Windows, so any Windows user that causes yamlfmt to
format yaml files will cause CI build failure because we build under Linux, and
the resulting format will differ, causing file changes during the pre-commit
step, causing build failure.

Therefore, this script avoids calling yamlfmt (via pre-commit) when running on
Windows.
"""
import platform
import subprocess
import sys

if platform.system() != "Windows":
    sys.exit(subprocess.call(["yamlfmt", *sys.argv[1:]]))

Then, modify the yamlfmt hook configuration in .pre-commit-config.yml to look like so:

  - repo: https://github.com/google/yamlfmt
    rev: v0.20.0
    hooks:
      - id: yamlfmt
        entry: python scripts/yamlfmt.py
        types_or: [yaml]
        exclude: ".*/vcr_cassettes/.*\\.yaml"

Notice that I've added entry to that section, which will call the script above, rather than calling yamlfmt directly.

Once you've added the new script and modified the pre-commit config, try running pre-commit run -a again (without setting SKIP) to see if it leaves the yaml files unchanged. If so, then please commit the new changes to this PR.

@ana-sher

Copy link
Copy Markdown
Contributor Author

It is a good way to solve it until google/yamlfmt#263 will be fixed! Worked as intended on my Windows machine, thank you @chuckwondo , added script to the PR.

@mfisher87 mfisher87 changed the title Prevent fixing symlink file Prevent formatting symlink file Nov 15, 2025
@chuckwondo

Copy link
Copy Markdown
Contributor

It is a good way to solve it until google/yamlfmt#263 will be fixed! Worked as intended on my Windows machine, thank you @chuckwondo , added script to the PR.

Fantastic! I'm glad that works.

I think the only thing left to do is add an entry to CHANGLOG.md indicating your contribution!

Under the "Unreleased" heading (near the top of CHANGELOG.md), there is a "Fixed" subheading. Please add the following item for your fix:

- Fix undesirable pre-commit changes when running on Windows
  ([#1143](https://github.com/nsidc/earthaccess/issues/1143)) (@ana-sher)

cc: @danielfromearth

@ana-sher

Copy link
Copy Markdown
Contributor Author

Done ✅ Thanks a lot for the mentoring on my first contribution, @chuckwondo!

@chuckwondo chuckwondo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome @ana-sher! Happy to welcome you as a new contributor!

@danielfromearth, since you were on the call too when we started debugging this, do you have any additional comments?

@danielfromearth

Copy link
Copy Markdown
Contributor

I think this looks great. The entry script is a clever way handle the issue!

If/when yamlfmt is updated so there are no longer differences between Operating Systems, this scripts/yamlfmt.py script might linger on yet then be unnecessary. I'm not sure if there is a way, but can we set something up so we would be alerted if we no longer need this script?

@chuckwondo

chuckwondo commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

I think this looks great. The entry script is a clever way handle the issue!

If/when yamlfmt is updated so there are no longer differences between Operating Systems, this scripts/yamlfmt.py script might linger on yet then be unnecessary. I'm not sure if there is a way, but can we set something up so we would be alerted if we no longer need this script?

I've subscribed to the yamlfmt issue, so I'll get notified of any future activity on the thread. If you want to do the same, then at least a few of us will see if/when it gets resolved.

If/when the yamlfmt issue if fixed, we can then open an issue to remove this newly added script along with the entry config added to .pre-commit-config.yml. It should be fairly obvious that if the entry is removed for the hook, that the script it refers to should also be removed.

@danielfromearth

Copy link
Copy Markdown
Contributor

Sounds good to me. I just now subscribed to that issue thread too 👀

@chuckwondo
chuckwondo merged commit fec2374 into earthaccess-dev:main Nov 17, 2025
7 checks passed
@github-project-automation github-project-automation Bot moved this to ✅ Done in earthaccess Mar 3, 2026
@mfisher87 mfisher87 removed this from earthaccess Mar 3, 2026
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.

[BUG] pre-commit on Windows modifying main branch files

4 participants