jorisroovers / gitlint

Linting for your git commit messages
http://jorisroovers.github.io/gitlint
MIT License
803 stars 99 forks source link

Ignore stdin by default in CI #423

Open scop opened 1 year ago

scop commented 1 year ago

One of the nice things of nox are good defaults, no matter if it's run in CI or not.

I thought it'd make sense for gitlint, too. Following the nox implementation, something like this could do the trick:

--- a/gitlint-core/gitlint/config.py
+++ b/gitlint-core/gitlint/config.py
@@ -84 +84 @@ class LintConfig:
-        self._ignore_stdin = options.BoolOption("ignore-stdin", False, ignore_stdin_description)
+        self._ignore_stdin = options.BoolOption("ignore-stdin", "CI" in os.environ, ignore_stdin_description)

Perhaps some additional patching for the test suite would be in order, as it's likely to run also in CI itself, but in principle. Doc updates obviously too.

jorisroovers commented 1 year ago

We can do this yes (not sure whether this is the right implementation, need to check).

However, is the implication here that gitlint doesn't work out of the box in your CI environment without specifying --ignore-stdin? I haven't looked at TTY/STDIN detection for a while, but there have been other reports, mostly on windows IIRC (#91 and #171). Would like to hear more...

sigmavirus24 commented 1 year ago

That's what concerns me as I haven't needed to ignore stdin at all

scop commented 1 year ago

I haven't come across a case where I would have actually needed it, rather just decided to follow gitlint's documentation on it being useful in (some?) CI :)

sigmavirus24 commented 1 year ago

So this should really be "improve documentation to indicate that this might be useful in CI"?

scop commented 1 year ago

Would be good to be more specific than that. I'm not sure what audience would such a change serve, and there wouldn't be much of a difference to the current situation. Alternatively, the note about CI could be removed altogether, I suppose people who need it will either find the option or not, no matter if CI is mentioned.

jorisroovers commented 1 year ago

Sorry for the delay - and thanks for opening issues and your doc fix PRs @scop, please keep them coming. We might not always move forward with everything, but the discussion and suggestions make gitlint better overall - greatly appreciated!

On topic:

So if we don't know of any CI environment where detection fails, I think I'm now undecided on the usefulness of this.

I agree with using sane defaults ("batteries included" ftw!) but you could also argue it's better to have a hard fail on TTY/STDIN detection (and people opening a bug) than it silently being bypassed because CI was set.

Alternatively, the note about CI could be removed altogether, I suppose people who need it will either find the option or not, no matter if CI is mentioned.

Are you referring to the mention of CI here, in the CLI flag? Or do you mean elsewhere in the gitlint docs? https://github.com/jorisroovers/gitlint/blob/1f8a16b462e03878fdd7b9cbaaed506c4bf05956/gitlint-core/gitlint/cli.py#L270

scop commented 1 year ago

Both. There's a mention at least in docs/configuration.md (marked "sometimes useful" there).