refactor(git-authors): add unit test for option --output
#1115
Conversation
* doc(git-authors): update option `--output` usage
bin/git-authors
Outdated
| no_email=1 | ||
| ;; | ||
| --output ) | ||
| if [[ -n $2 ]] && [[ $2 =~ ^[a-zA-Z] ]]; then |
There was a problem hiding this comment.
This validation is a breaking change since absolute paths paths and relative paths (starting with ./) will now error and fail. This includes paths like /dev/fd/30, which show up frequently in scripting.
There was a problem hiding this comment.
Thanks, I got this error and this is a missing I did not think about. For option --output, it looks supporting the path makes sense.
There was a problem hiding this comment.
Thank you for your modifications. But now, if the file begins with a non-ASCII character like á, the file will not be written to. I suggest removing this sort of check altogether (just keep the -z/-n, how it was before)
| fi | ||
| if [[ $# = 1 ]] && [[ "$1" = "--no-email" ]]; then | ||
| echo >&2 "--no-email option only can be used with --list | -l | --output" | ||
| exit 1 |
There was a problem hiding this comment.
Checks can be good, but I'm looking at a previous diff, and it seems like this is a breaking change? git authors --no-email seems to have written to file AUTHORS, excluding emails?
There was a problem hiding this comment.
The previous implement did not include this feature, just dump all authors has the email into AUTHORS. There is not breaking change to previous implement I thought now cause the behavior of git authors AUTHORS did not change. This condition is to forbid using --no-email with the default behavior when invoking git authors directly.
There was a problem hiding this comment.
I'm not sure I understand. On main:
$ ./bin/git-authors --no-email
$ # Produces AUTHORS without emailsThis branch:
./bin/git-authors --no-email
--no-email option only can be used with --list | -l | --outputThat is a breaking change.
There was a problem hiding this comment.
Also, please do not mark these conversations as resolved, the CONTRIBUTING mentions that is the responsibility of the reviewer. It makes it more difficult to track the conversation
| exit 0 | ||
| fi | ||
|
|
||
| while [[ $# -gt 0 ]]; do |
There was a problem hiding this comment.
[nit] Moving big sections around like the argument parsing makes it more difficult to read the diff. Can the argument parsing be moved back to where it was - at the top of the file?
| ;; | ||
| --output ) | ||
| if [[ -n $2 ]] && [[ $2 =~ ^(\.\/|\.\.\/|\/)?([a-zA-Z_](\/)?)+$ ]]; then | ||
| output=$2 |
There was a problem hiding this comment.
I notice the variables file and output behave and mean the same thing. Is it possible to be consistent and use the old, file variable instead of making a new one?
| fi | ||
| fi | ||
|
|
||
| if [[ $# = 0 ]]; then |
There was a problem hiding this comment.
There seems to be some duplication. There are two sections that have
authors >> "$somevariable"
exit 0Can they be combined into one? If they share the same variable name, it should be relatively straightforward
|
It is not the right time to do the refactor, do this again when the coverage is ok. |
Close #1100