Skip to content

Conversation

@dnephin
Copy link
Member

@dnephin dnephin commented Apr 12, 2017

Don't expose parser.Directive outside of the parser package
Remove duplication in parser.Parse() and make it easier to support #29161

dnephin added 4 commits April 12, 2017 14:48
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
Copy link
Member Author

dnephin commented Apr 12, 2017

@yongtang I think this will make #29161 cleaner. No need to pass in stderr, you can just return Warnings as part of Result() and handle them in the builder.

@yongtang
Copy link
Member

Thanks @dnephin I am on the road but will try to take a look and update #29161 in the next several days. (my flight is with united airline 😢)

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

@vdemeester vdemeester merged commit 700b480 into moby:master Apr 13, 2017
@dnephin dnephin deleted the refactor-builder-parser-directive branch April 13, 2017 18:49
@thaJeztah thaJeztah modified the milestones: 17.05.0, 17.06 Apr 15, 2017
@lowenna
Copy link
Member

lowenna commented May 17, 2017

@dnephin Hmmm, just seen this. By having processFirstLine separate from processLine, it means adding additional parse directives is very problematic (which is what I'm currently trying to do in another PR).

What was the rationale for splitting the first line out and not just treating every line as "just another line".

@lowenna
Copy link
Member

lowenna commented May 17, 2017

Oh, I see. It's the BOM handling. I'll figure it out :)

@dnephin
Copy link
Member Author

dnephin commented May 18, 2017

Ya, I was thinking about that case when I was making these changes, but didn't want to worry about it in case we never added another directive.

I think d.processLine() can be called in both cases, and the Directive() d.processingComplete = true can be set to true on the first non-directive line, instead of after the first line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants