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.

Sign up for GitHub

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

Jump to bottom

parser: handle parser directives #62

Merged
merged 2 commits into from
May 4, 2021
Merged

parser: handle parser directives #62

merged 2 commits into from
May 4, 2021

Conversation

m-ildefons
Copy link
Contributor

  • Add handling for parser directives (pragmas) to the parser:
    escape, index
  • Split off tests into separate modules
  • Version bump to 9.4.0

See also:
https://docs.docker.com/engine/reference/builder/#parser-directives

fixes: hadolint/hadolint#371
fixes: #48

lorenzo
lorenzo reviewed Apr 29, 2021
View reviewed changes
Copy link
Member

@lorenzo lorenzo left a comment

Choose a reason for hiding this comment

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

you're a hero, thanks for doing this!

@m-ildefons
Copy link
Contributor Author

One thing I forgot to mention is that this PR does not add functionality to prettyPrint for printing different escape characters, although it does add the ability to print the pragma. This means you can construct a valid AST that pretty-prints to an invalid Dockerfile

# syntax = `
RUN cmd a \
 && cmd b

In order to solve that, I'd need a way to have parameters with defaults in functions I think. What would be the preferred way to do that?

@m-ildefons
parser: handle parser directives
6b92272 
- Add handling for parser directives (pragmas) to the parser:
  `escape`, `index`
- Split off tests into separate modules
- Version bump to 9.4.0

See also:
https://docs.docker.com/engine/reference/builder/#parser-directives

fixes: hadolint/hadolint#371
fixes: hadolint#48
@m-ildefons
Copy link
Contributor Author

I've changed the pretty printer to be able to determine which escape character to use on the fly from the AST. This may be a breaking change though in the API. I've kept it in a separate commit so I can easily revert it, if breaking the API in this way is not an option.

@lorenzo
Copy link
Member

lorenzo commented May 4, 2021 •
edited
Loading

@m-ildefons I'm ok with breaking the API, but we need to bump the version to 10.0.0 can you do that?

@m-ildefons
pretty printer: detect escape character from AST
e872906 
- Add escape character detection logic to the pretty printer
- Replace the escape character on demand for printing
- Add tests ensuring correct default behaviour
- Version bump to 10.0.0 due to API change

When pretty printing, look ahead into the AST to be printed to determine
which escape character to choose. Then use that character during
printing.
This is possibly a major breaking change in the API, because `Arguments
Text` is no longer an instance of `Pretty`. Instead the pretty printer
expects all instructions to be of type `Instruction Text` and not
`Instruction args`. This allows the arguments to the instructions to be
printed with the correct escape character.
@lorenzo
Copy link
Member

lorenzo commented May 4, 2021

great job!

@lorenzo lorenzo merged commit 7e4bcee into hadolint:master May 4, 2021
@matsubara0507 matsubara0507 mentioned this pull request Jul 2, 2021
Merged
@mloskot mloskot mentioned this pull request Aug 2, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenzo lorenzo lorenzo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#escape=` is not supported Escape instruction is not handled
2 participants
@m-ildefons @lorenzo