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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Respect .styluaignore when explicitly passing in file names #751

Closed
mrcjkb opened this issue Sep 1, 2023 · 6 comments 路 Fixed by #765
Closed

Respect .styluaignore when explicitly passing in file names #751

mrcjkb opened this issue Sep 1, 2023 · 6 comments 路 Fixed by #765
Labels
enhancement New feature or request

Comments

@mrcjkb
Copy link

mrcjkb commented Sep 1, 2023

Hi 馃槃

I have a git pre-commit hook that calls stylua with file names (the staged files),
and I noticed that when passing in a file (rather than a directory), it is formatted, even if the file matches .styluaignore.

Is this intended behaviour? If so, I'm wondering if there's a way to modify this behaviour so that it works with pre-commit hooks, or if it would make sense to add a CLI flag?
(asking before I go ahead and open a PR)
@mrcjkb mrcjkb mentioned this issue Sep 1, 2023
Merged
1 task
@JohnnyMorganz
Copy link
Owner

Hey,
Yeah, passing a file directly will bypass ignore checks since we assume it was passed explicitly on purpose. Glob Filtering and .styluaignore both are only taken into account for directory traversal

@mrcjkb
Copy link
Author

mrcjkb commented Sep 2, 2023

Thanks for the info. Sounds reasonable. I'll see if I can fix this in the pre-commit hook.

@mrcjkb mrcjkb mentioned this issue Sep 2, 2023
Closed
dundargoc added a commit to dundargoc/neovim that referenced this issue Sep 10, 2023
@dundargoc
build(lint): use stylua without add_glob_target
ca7bc49 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
@dundargoc dundargoc mentioned this issue Sep 10, 2023
Merged
@dundargoc
Copy link
Contributor

dundargoc commented Sep 10, 2023 •
edited

@JohnnyMorganz is this the intended behavior? If so, could I try to persuade you to to rethink this behavior?

I'll spare you the boring details but long story short: our build system caches required work at a file level, meaning that we have to run stylua on each file individually if we want to avoid linting and formatting unmodified files. This procedure helps us save immense amounts of time by only doing work that is necessary. I suspect our build system isn't the only one with this behavior and that our situation isn't unique.

My proposal is instead that .styluaignore to always be respected regardless if given path is a file or directory. This way it .styluaignore becomes authoritative and, in my opinion, more consistent.

If you'd still like a way for stylua to be able to bypass .styluaignore then I personally think an explicit flag, e.g. --force, would serve the same purpose. This also covers the case where a user might want to format an entire directory marked by .styluaignore. In both cases (respect/ignore .styluaignore) the behavior for files and directories would be the same, which I think is desirable.

Thank you for hearing me out and thank you for stylua :)

@JohnnyMorganz
Copy link
Owner

I think it is reasonable to provide some way to continue respecting ignores when explicitly passing file names. For backwards compatibility reasons, it would probably have to be the inverse of your suggestion, instead providing a --respect-ignores or similar flag to enable this behaviour. I would prefer having a --force flag for this though... it makes more sense.

I was curious what the default behaviour of other tools are for this:

  • git add: respects .gitignore by default, with -f force flag
The following paths are ignored by one of your .gitignore files:
test.lua
hint: Use -f if you really want to add them.
hint: Turn this message off by running
hint: "git config advice.addIgnoredFile false"

  • prettier: respects .prettierignore by default - there seems to be no way to "force" it

  • black: does not respect includes by default. Requires a --force-exclude <TEXT> parameter to be passed to exclude a file even if it was passed explicitly to the command line

Each tool varies in its support

@JohnnyMorganz JohnnyMorganz added the enhancement New feature or request label Sep 10, 2023
@JohnnyMorganz JohnnyMorganz changed the title Take .styluaignore into account when passing in file names? Respect .styluaignore when explicitly passing in file names Sep 10, 2023
dundargoc added a commit to dundargoc/neovim that referenced this issue Sep 10, 2023
@dundargoc
build(lint): use stylua without add_glob_target
e15003e 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
dundargoc added a commit to dundargoc/neovim that referenced this issue Sep 10, 2023
@dundargoc
build(lint): use stylua without add_glob_target
32e1ee3 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
dundargoc added a commit to neovim/neovim that referenced this issue Sep 14, 2023
@dundargoc
build(lint): use stylua without add_glob_target
eecddd2 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
ur4ltz pushed a commit to ur4ltz/neovim-1 that referenced this issue Sep 15, 2023
@dundargoc @ur4ltz
build(lint): use stylua without add_glob_target
a144e55 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
@mrcjkb
Copy link
Author

mrcjkb commented Sep 15, 2023

I think --respect-ignores is fine.

As a user, if I'm running stylua on a file from the command line, I probably want to format that file, so --force might be a bit of an inconvenience.
For automated tasks (in a CI, or post-save in a text editor), it's trivial to add a --respect-ignores flag.

On the other hand, --force is safer - people have to be aware of --respect-ignores.

@JohnnyMorganz
Copy link
Owner

Turns out a while ago this was actually partially implemented by a user specifically for --stdin-filepath #495

Ideally this should also be behind the --respect-ignores flag, but changing it is a somewhat breaking change...

This was referenced Sep 16, 2023
Merged
Closed
dundargoc added a commit to dundargoc/neovim that referenced this issue Sep 27, 2023
@dundargoc
build(lint): use stylua without add_glob_target
1ab410b 
add_glob_target is our custom method to figure out whether a work needs
to be done or not. This works as expected most of the time, but causes a
problem with stylua.

Stylua makes the decision that if a file is explicitly passed to be
formatted, then it will format the file even if the file is set to be
ignored in .styluaignore. This behavior breaks add_glob_target with
seemingly no easy workaround.
More information: JohnnyMorganz/StyLua#751

Instead, what we can do is call stylua as you would in the command line.
This will make stylua work as expected. The downside is that we no
longer get a free "is this work necessary" detection, meaning that
stylua will be run each time `make lint` is called, regardless if it's
necessary or not. For longer lint tasks such as uncrustify and
clang-tidy this would be disastrous, but this is an acceptable tradeoff
since stylua is very quick.
@JohnnyMorganz JohnnyMorganz mentioned this issue Jan 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support --respect-ignores JohnnyMorganz/StyLua
3 participants
@mrcjkb @JohnnyMorganz @dundargoc