Skip to content

Change input flag to parser #410

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

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

jpreese
Copy link
Member

@jpreese jpreese commented Sep 22, 2020

This PR aims to change the --input flag to instead, be named --parser.

Reasoning:

The parser package exposes a number of different parsers, and these parsers are responsible for parsing different file formats. I feel its currently a little confusing to have conftest test --input tf, under the hood this just means to use the hcl2 parser, which we've done a mapping for. Instead, I think it makes more sense to say conftest test --parser hcl1 or conftest test --parser hcl2.

I do think theres value in being able to figure out which Parser to use, based on a file, which NewFromPath accomplishes.

This also keeps the language more consistent with what is really happening under the hood, figuring out which parser to use with the given input. As well as keeps the parser package focused on its parsers rather than specific implementations that use the language.

@jpreese jpreese requested a review from Blokje5 September 22, 2020 18:51
Signed-off-by: John Reese <john@reese.dev>
@jpreese jpreese force-pushed the change-input-parser branch 2 times, most recently from b314e48 to 037ecfa Compare September 22, 2020 19:03
Signed-off-by: John Reese <john@reese.dev>
@jpreese jpreese force-pushed the change-input-parser branch from 037ecfa to f272372 Compare September 22, 2020 20:41
Copy link
Collaborator

@Blokje5 Blokje5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It is risky to change the flag, but given that we are not at v1 yet I do not mind creating a backwards incompatible change.

As a potential option we could leave both flags in for a single release and print a deprecation warning on using the --input flag. That way users have a bit more time to adapt to the change. But i'll leave that up to you.

@jpreese
Copy link
Member Author

jpreese commented Sep 23, 2020

Thanks for the review! I had the same concern as well, but being that I suspect this is a low-usage flag and being pre-v1, I didn't think it worth it to maintain backwards compatibility.

@jpreese jpreese merged commit b5bad18 into open-policy-agent:master Sep 23, 2020
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.

2 participants