Skip to content

WIP: - Parse Host, Port, Image and Tag with regular expressions #64

Open
bougar wants to merge 5 commits into
optiopay:masterfrom
bougar:master
Open

WIP: - Parse Host, Port, Image and Tag with regular expressions #64
bougar wants to merge 5 commits into
optiopay:masterfrom
bougar:master

Conversation

@bougar

@bougar bougar commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

No description provided.

@hashmap

hashmap commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

the first version used regexs, we switched to custom parsing later on 876ecd8

@bougar

bougar commented Oct 26, 2017

Copy link
Copy Markdown
Contributor Author

What's the point in using custom implementation instead regex? :D
Sorry by the inconveniencies, I did not see that commit. Anyway, I mind regex like:
ImageRegexp = match((?:[/])(?P<name>[a-zA-Z0-9/]*)[:@]?(?P<tag>.*)?)

@hashmap

hashmap commented Oct 27, 2017

Copy link
Copy Markdown
Contributor

One pass instead of multiple matches as it was before, you approach looks interesting

Comment thread docker/docker.go
state = stateTag
}
nameParts = append(nameParts, part)
if !utils.ImageOnlyRegexp.MatchString(string(qname)) || strings.HasPrefix(qname, "localhost/") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type of qname already is string, why still need to convert it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ups, I thought it was a byte. There is no point in convert string into string XD

@supereagle supereagle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The approach is good, which makes it much clearer.

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.

3 participants