Skip to content

Nonzero exit code when diff is not clean#9

Merged
yannickhilber merged 1 commit intomasterfrom
3_nonzero_exit_code_diff
Feb 22, 2023
Merged

Nonzero exit code when diff is not clean#9
yannickhilber merged 1 commit intomasterfrom
3_nonzero_exit_code_diff

Conversation

@yannickhilber
Copy link
Copy Markdown
Contributor

Store diff prints to text buffer and exit program with code 2 when the diff is not empty.

@yannickhilber yannickhilber requested a review from ruuda February 16, 2023 13:33
@yannickhilber yannickhilber force-pushed the 3_nonzero_exit_code_diff branch 2 times, most recently from 493ed21 to 1df4df5 Compare February 16, 2023 13:43
Copy link
Copy Markdown
Contributor

@ruuda ruuda left a comment

Choose a reason for hiding this comment

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

This works, but it is also quite invasive. What I would do instead is to keep track of something like has_changes = False in main, then make print_diff return whether there was a change, then we can do something like

has_changes |= print_diff(...)

and then at the end we can check has_changes.

(Be careful with or and side effects as or short-circuits; if the left-hand side is true then it never executes the right-hand side. |= does not short-circuit.)

@yannickhilber
Copy link
Copy Markdown
Contributor Author

You're right this solution is cleaner. It required to track the changes on print_team_members_diff where we instantiate a Diff and when the we check if the organizations matches.

I needed to add a boolean variable in print_diff and print_team_members_diff to go through all the if statements. I called it has_diff to match with the function names and not to confuse it with has_changes in the main function.

@yannickhilber yannickhilber reopened this Feb 17, 2023
Copy link
Copy Markdown
Contributor

@ruuda ruuda left a comment

Choose a reason for hiding this comment

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

This looks good! Only please make sure the file typechecks, you can typecheck it with mypy --strict.

Comment thread main.py Outdated
Comment thread main.py Outdated
@yannickhilber yannickhilber force-pushed the 3_nonzero_exit_code_diff branch from 962c84b to 5381cfe Compare February 21, 2023 12:45
@yannickhilber
Copy link
Copy Markdown
Contributor Author

This looks good! Only please make sure the file typechecks, you can typecheck it with mypy --strict.

Thanks I was not aware of this tool. I will use it in the future.

@ruuda
Copy link
Copy Markdown
Contributor

ruuda commented Feb 21, 2023

Hmm something weird happened, this pull request now touches LICENSE and nixpkgs-pinned.nix, and I see some commits that I authored but you committed ... I think you rebased too much. You could fix this with

git fetch origin
git rebase --onto origin/master 3_nonzero_exit_code_diff^ 3_nonzero_exit_code_diff

@yannickhilber yannickhilber force-pushed the 3_nonzero_exit_code_diff branch from 5381cfe to 4975a19 Compare February 22, 2023 07:13
@yannickhilber
Copy link
Copy Markdown
Contributor Author

Hmm something weird happened, this pull request now touches LICENSE and nixpkgs-pinned.nix, and I see some commits that I authored but you committed ... I think you rebased too much. You could fix this with

git fetch origin
git rebase --onto origin/master 3_nonzero_exit_code_diff^ 3_nonzero_exit_code_diff

This is not the first time that I rebased too much and past commits are never added. I tried to reproduce what happened by adding a commit and squashing it to the previous with git rebase -i HEAD~8 but it didn't work 😕

Thanks a lot for the fix, it worked perfectly !

@yannickhilber yannickhilber merged commit 6fc4782 into master Feb 22, 2023
@yannickhilber yannickhilber deleted the 3_nonzero_exit_code_diff branch February 22, 2023 07:23
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.

2 participants