-
Notifications
You must be signed in to change notification settings - Fork 102
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
Initial implementation of 'ignore-stdin' flag #89
Conversation
This feature is beneficial when running in automated environments liek CI servers. Current heuristic fails in some conditions. With this flag enabled we can simply always ignore stdin and fall back to using target repo. Closes jorisroovers#56
@jorisroovers any comments? |
@jorisroovers bump :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay, life get's in the way :-) I'll try to circle back faster on your reply.
This looks really good! Just a minor suggestion in the code wrt the flag precedence.
Re: flag name, I agree it might be a little confusing, especially for the most common use-case which is linting the local repo (without specifying the --target
flag). In this case, the word 'target' is likely to trip users up.
Having said that, I spent a few min brainstorming (see below) and couldn't immediately think of a better name. If you don't know of anything else, then I'm good to move ahead with the suggested force-target-repo
.
# target: might be confusing for most common use-case of reading repo in current working dir
force-target-repo
force-use-target
# local: implies we always read the local repo, even if a --target is specified (we don't want this)
force-local-repo
# git: doesn't sound right to me for some reason
force-git-repo
force-use-git-repo
force-read-git-repo
gitlint/cli.py
Outdated
if msg_filename: | ||
LOG.debug("Attempting to read from --msg-filename.") | ||
gitcontext = GitContext.from_commit_msg(ustr(msg_filename.read())) | ||
elif stdin_input: | ||
LOG.debug("No --msg-filename flag. Attempting to read from stdin.") | ||
elif stdin_input and not lint_config.force_target_repo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering whether it makes sense to make lint_config.force_target_repo
the first option in the if block, like so:
if lint_config.force_target_repo:
LOG.debug("--force-target-repo flag specified. Reading from local repository.")
gitcontext = GitContext.from_local_repository(lint_config.target, ctx.obj[2])
elif msg_filename:
LOG.debug("Attempting to read from --msg-filename.")
gitcontext = GitContext.from_commit_msg(ustr(msg_filename.read()))
elif stdin_input:
LOG.debug("No --msg-filename flag. Using data passed via stdin.")
else:
# ...
Reasoning is that if a user uses a flag called '--force-xxx', I think they'd expect the force word to imply that it to take precedence over everything else (incl --msg-filename).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just --ignore-stdin switch is better idea? In that case if statements do not have to be rearranged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea! Much more intuitive CLI argument name too!
Would you mind updating the code (or submitting new PR, whatever is easier)?
This is great, thank you! I'll wrap a few other things over the next few days (max 1 week) and then push this out as part of the 0.12.0 release. Thanks! |
This feature is beneficial when running in automated environments
liek CI servers. Current heuristic fails in some conditions.
With this flag enabled we can simply always ignore stdin
and fall back to using target repo.
Closes #56
Needs further discussion, especially regarding actual naming of flag (maybe disregard_stdin would be better idea), test coverage and docs examples.
Feature itself is kind of "meh, boring" :) .