Feat 451 add commit args #621

SCrocky · 2022-11-26T11:05:41Z

Description

Added extra argument mechanic for cz commit command so that all arguments after a -- will be treated as extra args.
Example: You can now sign commits with cz c -- -S

Checklist

Add test cases to all the changes you introduce
Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
Test the changes on the local machine manually
Update the documentation for the changes

Steps to Test This Pull Request

Additional context

This PR should close issue 451

Lee-W

Sorry for late reviewing. The overall idea looks good to me! But we'll need doc and unit test for this one. After this is merged, I think we can have one branch removing the original signoff and send that PR to v3 branch

SCrocky · 2022-12-17T03:29:37Z

Sorry for late reviewing. The overall idea looks good to me! But we'll need doc and unit test for this one. After this is merged, I think we can have one branch removing the original signoff and send that PR to v3 branch

Sounds good, I'll get on the tests next.

woile · 2023-04-28T13:34:25Z

Any update on this? We'd appreciate having the tests and rebasing 🙏🏻

SCrocky · 2023-04-28T16:39:50Z

I have some time this weekend. I'll try to finish it then.

SCrocky · 2023-05-15T13:11:14Z

Done rebasing, added both tests and doc.

SCrocky · 2023-05-15T13:12:06Z

Done reading, added both tests and doc.
@woile @Lee-W

Lee-W

I am thrilled about this wonderful feature! I was able to spare some time to examine it. I have just provided a few comments and suggestions.

Lee-W · 2023-06-03T04:19:23Z

commitizen/commands/commit.py

@@ -91,7 +91,11 @@ def __call__(self):
        signoff: bool = self.arguments.get("signoff")

        if signoff:
+            out.warn("signoff mechanic is deprecated, please use `cz commit -- -s` instead.")


@woile Do you think we should add a list somewhere so we won't forget to deprecate stuff in the next release?

@SCrocky In this implementation, if we're using the original signoff feature, we won't be able to use extra_cli_args.. Should we implement it as something like the following? (the warning is still needed)

extra_cli_args = "" if signoff: extra_cli_args = "-s "

Ah, I see it. Thanks for that.

Would this work for you?

if signoff: out.warn("signoff mechanic is deprecated, please use `cz commit -- -s` instead.") extra_args = "-s " + self.arguments.get("extra_cli_args") else: extra_args = self.arguments.get("extra_cli_args") c = git.commit(m, extra_args=extra_args)

I might try something like

extra_args = self.arguments.get("extra_cli_args", "") if signoff: out.warn("signoff mechanic is deprecated, please use `cz commit -- -s` instead.") extra_args += " -s" c = git.commit(m, extra_args=extra_args)

But one thing I'm thinking is whether we should handle cz commit -s -- -s

I tested it and doesn't seem to be an issue with the git command:

>>> git commit -s -s -m "test commit" [master (root-commit) b864659] test commit 1 file changed, 0 insertions(+), 0 deletions(-)

But it is a bit messy. Warning or error?

If it doesn't cause an error, I'm good with either silently passing or warning.

Lee-W · 2023-06-03T04:23:26Z

commitizen/commands/commit.py

            c = git.commit(m, "-s")
+
+        if self.arguments.get("extra_cli_args"):


Do we need this if-else block? Could we handle the case in git.commit() when extra_cli_args is not provided?

Lee-W · 2023-06-03T04:26:33Z

docs/commit.md

+```
+cz commit -commitizen-args -- -git-cli-args
+```


Suggested change

```

cz commit -commitizen-args -- -git-cli-args

```

```sh

cz commit -- <git-cli-args>

# e.g., cz commit --dry-run -- -a -S

Can we keep commitizen-args ? It provides clarity IMO.

cz commit <commitizen-args> -- <git-cli-args> # e.g., cz commit --dry-run -- -a -S

Sure 🙂 Looks better 👍

Lee-W · 2023-06-03T04:28:59Z

commitizen/cli.py

+            raise InvalidCommandArgumentError(
+                f"Invalid commitizen arguments were found before -- separator: `{' '.join(unknown_args[:pos])}`. "
+            )
+        extra_args = " ".join(unknown_args[1:])


We might need to handle the case that the user use -- without actually providing an argument. e.g., cz commit --

Sure, how should we deal with it?

Warning or error?

Warning should be enough in this case 👍

mesaglio · 2023-09-23T01:20:49Z

Is this wait something? I think -S flag its good to use for security reasons.

SCrocky · 2023-09-24T11:59:37Z

I've fixed rebased to master, fixed all the previous issues, added/edited tests and added some documentation.

@Lee-W ready for Round 2?

codecov · 2023-09-24T12:01:08Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
commitizen/__version__.py	`100.00% <100.00%> (ø)`
commitizen/commands/bump.py	`97.66% <100.00%> (+0.01%)`	⬆️
commitizen/commands/commit.py	`98.68% <100.00%> (+0.09%)`	⬆️
commitizen/config/json_config.py	`100.00% <100.00%> (+6.45%)`	⬆️
commitizen/config/toml_config.py	`100.00% <100.00%> (ø)`
commitizen/config/yaml_config.py	`100.00% <100.00%> (ø)`
commitizen/defaults.py	`98.38% <100.00%> (-1.62%)`	⬇️
commitizen/providers/__init__.py	`100.00% <100.00%> (ø)`
commitizen/providers/base_provider.py	`100.00% <100.00%> (ø)`
commitizen/providers/cargo_provider.py	`100.00% <100.00%> (ø)`
... and 8 more

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Lee-W

@woile @noirbizarre Might need some time to take a deeper look, but I think most of the parts are good.

Lee-W · 2023-10-01T14:13:06Z

commitizen/git.py

+    args: str = "",
+    extra_args: str = "",


Would it be possible for us to combine these 2? Not sure what's the difference between them

extra_args are arguments that we just want to pass to git transparently (like -S).

Having the extra_args argument separate helps with testing and clarity.

For example:extra_args has to start with "--" and contain at least one flag behind it. (e.g. "-- -S").

If this is contained within the args argument, then it becomes a little harder to know whether or not any extra args were passed.

Would it be possible for us to combine these 2?

git.commit is only called with extra_args in commands/commit.py __call__ method where it doesn't have any args.

So technically, if I'm not missing anything, I could just use the args argument to pass the extra_args.

Not sure what's the difference between them

I'll add a Docstring

So technically, if I'm not missing anything, I could just use the args argument to pass the extra_args.

Yep, I think that's what we should do. Thanks for clarifying!

… for `cz commit` command Additional edits were made in class `commitizen.commands.Commit` and `commit` func from `commitizen.git` Closes commitizen-tools#451

Signoff dedicated code is no longer necessary & removed #TODO as tests show identical args and extra-args are not an issue.

… commitizen args from git commit cli args

…ests

…and' warning

Lee-W

Thanks, @SCrocky, for your contribution! This really helps a lot. Could you please make it ready for review?

@woile @noirbizarre I'm planning on merging it these days. Please let me know if you want to take a look

SCrocky · 2023-10-03T14:32:19Z

Thanks, @SCrocky, for your contribution! This really helps a lot.

No problem! I'm really glad to have finally finished it. 🙏

Could you please make it ready for review?

Is this addressed to me? I'm sorry, I'm not sure what the remaining steps are. Is there anything else I need to do?

Lee-W · 2023-10-03T14:45:05Z

Thanks, @SCrocky, for your contribution! This really helps a lot.

No problem! I'm really glad to have finally finished it. 🙏

Could you please make it ready for review?

Is this addressed to me? I'm sorry, I'm not sure what the remaining steps are. Is there anything else I need to do?

Ah, just found out I can do it as well

Lee-W reviewed Dec 15, 2022

View reviewed changes

Lee-W added the in progress label Jan 27, 2023

SCrocky force-pushed the feat-451-add-commit-args branch from 600f96a to 75c9f56 Compare May 13, 2023 05:57

Lee-W reviewed Jun 3, 2023

View reviewed changes

Lee-W added pr-status: wait-for-modification and removed in progress labels Jun 23, 2023

github-actions bot added the pr-status: wait-for-review label Sep 23, 2023

SCrocky force-pushed the feat-451-add-commit-args branch from 3d8d27c to 0bf5c8a Compare September 24, 2023 11:58

Lee-W reviewed Oct 1, 2023

View reviewed changes

Sebastien Crocquevieille and others added 8 commits October 3, 2023 02:41

feat(cli.py): Added support for extra git CLI args after -- separator…

adfdfb1

… for `cz commit` command Additional edits were made in class `commitizen.commands.Commit` and `commit` func from `commitizen.git` Closes commitizen-tools#451

refactor(Commit): Added deprecation on git signoff mechanic

5888f37

Signoff dedicated code is no longer necessary & removed #TODO as tests show identical args and extra-args are not an issue.

test(cli.py): Added tests for -- argument notation raises

142094e

docs(docs/commit.md): Added documentation for -- syntax to separate…

081332d

… commitizen args from git commit cli args

refactor: Code Review - round 1 changes

0abb01e

refactor(extra_args): Fixed broken code due to rebase and finalized t…

8df5c94

…ests

style(extra_args): Added newlines and WARN to 'Incomplete commit comm…

4baecd6

…and' warning

refactor(git.py): Removed 'extra_args' from git.commit

5159a62

SCrocky force-pushed the feat-451-add-commit-args branch from ac3f3d9 to 5159a62 Compare October 2, 2023 18:42

Lee-W approved these changes Oct 3, 2023

View reviewed changes

Lee-W added pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check and removed pr-status: wait-for-review pr-status: wait-for-modification labels Oct 3, 2023

Lee-W marked this pull request as ready for review October 3, 2023 14:34

Lee-W merged commit 12b214e into commitizen-tools:master Oct 17, 2023
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat 451 add commit args #621

Feat 451 add commit args #621

SCrocky commented Nov 26, 2022

Lee-W left a comment

SCrocky commented Dec 17, 2022

woile commented Apr 28, 2023

SCrocky commented Apr 28, 2023

SCrocky commented May 15, 2023 •

edited

Loading

SCrocky commented May 15, 2023

Lee-W left a comment

Lee-W Jun 3, 2023

Lee-W Jun 3, 2023

SCrocky Jun 3, 2023

Lee-W Jun 4, 2023

Lee-W Jun 4, 2023

SCrocky Jun 4, 2023 •

edited

Loading

Lee-W Jun 4, 2023

Lee-W Jun 3, 2023

Lee-W Jun 3, 2023

SCrocky Jun 3, 2023

Lee-W Jun 4, 2023

Lee-W Jun 3, 2023

SCrocky Jun 3, 2023

Lee-W Jun 4, 2023

mesaglio commented Sep 23, 2023 •

edited

Loading

SCrocky commented Sep 24, 2023

codecov bot commented Sep 24, 2023 •

edited

Loading

Lee-W left a comment

Lee-W Oct 1, 2023

SCrocky Oct 1, 2023

Lee-W Oct 2, 2023

SCrocky Oct 2, 2023

Lee-W left a comment

SCrocky commented Oct 3, 2023

Lee-W commented Oct 3, 2023

		c = git.commit(m, "-s")

		if self.arguments.get("extra_cli_args"):

-```
-cz commit -commitizen-args -- -git-cli-args
-```
+```sh
+cz commit -- <git-cli-args>
+# e.g., cz commit --dry-run -- -a -S

Feat 451 add commit args #621

Feat 451 add commit args #621

Conversation

SCrocky commented Nov 26, 2022

Description

Checklist

Steps to Test This Pull Request

Additional context

Lee-W left a comment

Choose a reason for hiding this comment

SCrocky commented Dec 17, 2022

woile commented Apr 28, 2023

SCrocky commented Apr 28, 2023

SCrocky commented May 15, 2023 • edited Loading

SCrocky commented May 15, 2023

Lee-W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCrocky Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mesaglio commented Sep 23, 2023 • edited Loading

SCrocky commented Sep 24, 2023

codecov bot commented Sep 24, 2023 • edited Loading

Codecov Report

Lee-W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lee-W left a comment

Choose a reason for hiding this comment

SCrocky commented Oct 3, 2023

Lee-W commented Oct 3, 2023

SCrocky commented May 15, 2023 •

edited

Loading

SCrocky Jun 4, 2023 •

edited

Loading

mesaglio commented Sep 23, 2023 •

edited

Loading

codecov bot commented Sep 24, 2023 •

edited

Loading