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

feat(ng-dev/format): add staged files back #405

Closed

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Feb 12, 2022

change the staged command so that after formatting it also adds
formatted files back to the staging area (unless --check is used
of course)


I just wanted to improve one small thing for development on the angular repo, basically when I create a commit and the pre-commit git hook formats the staged files, those changes aren't actually included in the commit, so I usually need to re-add the formatted files, update my commit and force push it.

I think that it is pretty inconvenient and it would just make sense for the hook to both format them and add them back to the staging area (I don't think this could be an unwanted behavior since the ci checks would anyways required the user to update those files).

So my idea is to just extend this:
https://github.com/angular/angular/blob/444354855b1e19ef383cf63088cbb31d59a14593/.husky/pre-commit#L5
to use this new staged-re command instead


Alternatively I could have added a flag as --re-stage or whatever but that wouldn't have made sense for the other format commands so I figured just a separate command could make sense


Also sorry for my boldness on opening a random PR here, I hope it's fine, if it is a bad idea you can always just shut it down anyways 🥲 🙇

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

I think this is a cool feature, definitely would have saved me some time in the past.

Just not sure about the API. I could imagine it being an optional flag for just staged.

Generally I believe formatted files should be staged automatically as this may interfere with the process of creating e.g. multiple commits based on certain criteria

cc. @josephperrott as the author of this command

@dario-piotrowicz
Copy link
Contributor Author

I think this is a cool feature, definitely would have saved me some time in the past.

Thanks a lot!!! 😍

Just not sure about the API. I could imagine it being an optional flag for just staged.

I totally agree with you, I just did it by adding a new command because I am not too familiar with yargs and thought adding options to those chained commands was not possible 😅 🙇 , sorry I looked into it and updated the thing to have a --add flag for the addition of the formatted files to the staging area

thanks a lot for making me look into it! (and sorry for the first version 🙇)

PS: to keep the description short I put "add the potentially formatted files" instead of saying that this option gets ignored if you use --check, what do you think? is it clear enough? 🙂

Generally I believe formatted files should be staged automatically as this may interfere with the process of creating e.g. multiple commits based on certain criteria

Same ☺️👍

@dario-piotrowicz
Copy link
Contributor Author

Ah, also, there are some circle ci failures, I can't tell what they are 😓 , please let me know if I need to do something to fix them 🙂

@devversion
Copy link
Member

No worries at all! I think both were valid approaches. It really comes down to preference and what might be best DX. I would definitely like to hear from @josephperrott about his thoughts.

PS: to keep the description short I put "add the potentially formatted files" instead of saying that this option gets ignored if you use --check, what do you think? is it clear enough?

Interestingly I think there is a way in Yargs to indicate that something conflicts with another option. You may want to try that. Also should this option be applied to all sub-commands for ng-dev format? (that might be a good question to answer as well; e.g. yarn ng-dev format changed --add

For CI: You need to run yarn update-generated-files because we check-in some bundles into the repository for Github actions. Some github actions rely on code that you modified transitively.

ng-dev/format/cli.ts Outdated Show resolved Hide resolved
const executionCmd = check ? checkFiles : formatFiles;
const allStagedFiles = GitClient.get().allStagedFiles();
process.exitCode = await executionCmd(allStagedFiles);
if (!check && add && process.exitCode === 0) {
GitClient.get().stageFiles(allStagedFiles);
Copy link
Member

Choose a reason for hiding this comment

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

So the question we "have to answer" is if this should just stage, or if it should amend the commit to include these changes.

The pre-commit githook which we use (and is the first of the commit related hooks), allows you to "inspect the snapshot that's about to be committed." Since its a snapshot, if you modify the files they are not included in the snapshot that goes into the commit. This is actually the reason I didn't create it this way to begin with. With what you have here, you still end up with the files just included in your clients staged files but the original files still in the commit you create. Or at least this was my understanding at the time, but I am not certain as its been a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that pre-commit hooks do allow us to add the files to the staging area before the commit happens, thus including them in the commit itself

I'll check it out and let you know how it goes 🙂👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just adding them to the staging area does seem to do the trick for me:
Peek 2022-02-14 23-18

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Feb 14, 2022

@devversion, I am not sure about putting the --add to all the format commands, I feel like it makes most sense for the format staged, but if you want I can also add it to the others 🤔 🤷‍♂️

Ah, and thanks a lot for the yarn update-generated-files, I'll try that out! 🙂

ng-dev/utils/git/git-client.ts Outdated Show resolved Hide resolved
const executionCmd = check ? checkFiles : formatFiles;
const allStagedFiles = GitClient.get().allStagedFiles();
process.exitCode = await executionCmd(allStagedFiles);
if (!check && add && process.exitCode === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

@devversion what do you think of just always re-staging the files when format staged is executed? We would not run it during a check obviously.

Suggested change
if (!check && add && process.exitCode === 0) {
if (!check && process.exitCode === 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've applied the change since I was anyways having issues passing the --add flag to the command in the bash pre-commit script (strangely enough it worked perfectly if called directly in the terminal, also --check worked just fine in the script 🤷‍♂️)

if we don't want it I can always revert the change anyways 🙂 (and keep digging as to why --add wouldn't be recognized 😅 )

Anyways yeah I do agree that it could make sense to have this behavior, I am just wondering, is there any case in which this could be undesirable? (keeping in mind that we do still have the --check anyways)

Copy link
Member

Choose a reason for hiding this comment

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

@josephperrott Sorry, didn't realize I got mentioned here again. I'm good with always staging (when not checking).

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Feb 14, 2022

Reminder

  • if/when the PR gets approved and it's ready to go I need to update the commit message

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM

ng-dev/utils/git/git-client.ts Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
change the staged command so that after formatting it also adds
formatted files back to the staging area (unless --check is used
of course)
@dario-piotrowicz dario-piotrowicz changed the title feat(ng-dev/format): add staged-re command feat(ng-dev/format): add staged files back Feb 17, 2022
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker domain: pr automation labels Feb 17, 2022
@josephperrott
Copy link
Member

This PR was merged into the repository by commit 74b89a4.

@devversion
Copy link
Member

Thank you @dario-piotrowicz

@dario-piotrowicz dario-piotrowicz deleted the format-staged-add branch February 17, 2022 22:17
@dario-piotrowicz
Copy link
Contributor Author

Thank you @devversion and @josephperrott for the awesome reviewing 😄👍


(PS: please let me know if I need to do anything further to introduce this in the Angular repo 🙂)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker domain: pr automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants