Skip to content
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

cli: read optional config from YAMLLINT_CONFIG #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbergstroem
Copy link

This is a "minor" abi break (?) since empty configs are disregarded instead of failing.

Fixes: #107

This is a "minor" abi break since empty configs are
disregarded instead of failing.

Fixes: adrienverge#107
@coveralls
Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage decreased (-0.1%) to 98.043% when pulling e52c1ac on jbergstroem:feature/environment-read into e4e99f0 on adrienverge:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.043% when pulling e52c1ac on jbergstroem:feature/environment-read into e4e99f0 on adrienverge:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.043% when pulling e52c1ac on jbergstroem:feature/environment-read into e4e99f0 on adrienverge:master.

@jbergstroem
Copy link
Author

So, my minor logic change now left us with a few uncovered branches. Suggestions on how to tackle this? Add tests? Different route for handling the parsing of environment variables?

@jbergstroem
Copy link
Author

@adrienverge any input on above?

@adrienverge
Copy link
Owner

Hi Johan, thanks, this looks good.

The API break is not problematic in my opinion, as empty config data resulted in an error.

Could you explain your comment on the few uncovered branches? I didn't get it.

About tests:

  • test_run_with_empty_config() should be changed to run without error (on an existing file), as this is the new behaviour.
  • Please add a case for checking that -d option overrides the environment.

@adrienverge
Copy link
Owner

Also, using -d '' should be a way to cancel the use of $YAMLLINT_CONFIG, what do you think? We would need a test for that.

@adrienverge
Copy link
Owner

@jbergstroem if you want to continue on this pull request, using YAMLLINT_CONFIG_DATA instead of YAMLLINT_CONFIG would be consistent with the new feature from #255 (review).

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.

Override config via environment
3 participants