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

Allow explicitly adding other changed files without warning #770

Closed
taion opened this issue Jan 20, 2020 · 12 comments · Fixed by #772
Closed

Allow explicitly adding other changed files without warning #770

taion opened this issue Jan 20, 2020 · 12 comments · Fixed by #772
Labels

Comments

@taion
Copy link

taion commented Jan 20, 2020

Description

I have a lint-staged pipeline that looks sort of like this, triggered by a Husky pre-commit hook:

{
  "*.js": [
    "eslint --fix",
    "npm run update-schema",
    "git add data/schema.graphql"
  ]
}

The idea here is that the update-schema script will update data/schema.graphql, and I want the commit to include synced changes.

The changed behavior around git add in v10.0.0 presents some challenges. The git add data/schema.graphql now generates a CLI warning that I can't disable. However, if I remove it, then changes to data/schema.graphql generated by the update-schema script no longer get added to my commit.

I can avoid the warning by doing something like () => 'git add data/schema.graphql', but that would require me to add an explicit JS config file, instead of just inlining my config inside my package.json.

Is there a good way, then, to support this use case while still using JSON configuration?

Environment

  • OS: Mac OS Mojave
  • Node.js: v12.14.1
  • lint-staged: v10.0.0
@iiroj
Copy link
Member

iiroj commented Jan 20, 2020

The git add warning checks if the command simply includes that text. Since the previous recommended approach was to have git add string as the full command, maybe we can just limit the warning to that exact match instead of a substring match.

What do you think?

@taion taion changed the title Allow explicitly adding other changed files in warning Allow explicitly adding other changed files without warning Jan 20, 2020
@iiroj
Copy link
Member

iiroj commented Jan 20, 2020

Another possibility would be to always run git add . internally. This way you wouldn't have to have it in your config. This had other issues though; what do you think @okonet?

@okonet
Copy link
Collaborator

okonet commented Jan 20, 2020

What if we allow the git add in the tasks. We can allow disabling the warning I think.

@iiroj
Copy link
Member

iiroj commented Jan 20, 2020

It actually is allowed, the warning doesn’t prevent it from running. A config flag could disable the warning, but I’m leaning on showing it only when commands.some(cmd => cmd === ’git add’) (Vs now it’s cmd.includes).

@taion
Copy link
Author

taion commented Jan 20, 2020

That could work.

The slightly awkward thing is that it means that, in some sense, I'd be running git add on all my other files twice – once from the git add <whatever> command, and once from the auto-add logic.

I'm not sure if there's a good way around that.

I could get around all this with a lint-staged.config.js file, but it seems like this sort of "generate a schema file" use case is common enough that it ought to be supported without the bail-out to a custom function command.

@iiroj
Copy link
Member

iiroj commented Jan 20, 2020

To clarify, when lint-staged starts it will gather a list of staged files. After running the tasks, it will run git add on that list so any modifications get added. This means that edits to a previously-unrelated file won’t get added into the commit.

Assuming data/schema.graphql wasn’t staged, the git add command should only run once over it.

@taion
Copy link
Author

taion commented Jan 20, 2020

I meant all the other files. That is, if I change src/foo.js, then, with v10, my setup above will run git add data/schema.graphql src/foo.js, and then (unless I'm misunderstanding), lint-staged itself will again run git add src/foo.js.

Obviously not a big deal, but ...

@iiroj
Copy link
Member

iiroj commented Jan 20, 2020

Ah yes, you are correct.

@taion
Copy link
Author

taion commented Jan 20, 2020

On the other hand, adding a new API primitive or special behavior for just git add commands probably isn't worth it.

Would it work to make the behavior in 083b8e7 optional? i.e. make it so that the git add can pick up extra changed files?

@iiroj
Copy link
Member

iiroj commented Jan 21, 2020

Can you live with the warning for a while, @taion? I think there are multiple valid approaches and we need to figure out the best.

In my opinion having the automatic git add step is a good improvement.

This displayed warning was meant as a notification to migrate the 99 % simple cases that have followed the project's examples, but it might be unnecessary to display anything (since it's in the CHANGELOG).

@taion
Copy link
Author

taion commented Jan 21, 2020

Sure, that’s for me for now.

@okonet
Copy link
Collaborator

okonet commented Jan 22, 2020

🎉 This issue has been resolved in version 10.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants