👻 Enhance regex patterns #1745
👻 Enhance regex patterns #1745
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
cncf-cla: yes
|
I used 👻 as this is a minor change that I don't think is worth to include in changelogs, but the verifier is not understanding it despite being mentioned in CONTRIBUTING.md. Guess I could change it to ✨ if needed. |
| dns1123SubdomainFmt string = dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*" | ||
| dns1035LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?" | ||
| dns1035LabelFmt string = "[a-z](?:[-a-z0-9]*[a-z0-9])?" |
There was a problem hiding this comment.
For we not add ore one dependency to the project we decided to copy and paste the implementation to validate.
We should not change this file unless it was changed in the k8s.io/apimachinery
See:
kubebuilder/pkg/internal/validation/dns.go
Lines 24 to 26 in 1759529
There was a problem hiding this comment.
Could you please revert the changes here?
There was a problem hiding this comment.
I would agree with you if the code was copy-pasted, but it has been already modified. You can check it. So Convering that capture group into a non-capture group tells the future us reading the code that we are not using that capture group to take a substring out of the regex. It also avoid having to allocate some variables but the performance increase will be very small.
There was a problem hiding this comment.
Catcher. I would prefer if possible we keep a copy and paste of https://github.com/kubernetes/apimachinery/blob/v0.18.6/pkg/util/validation/validation.go whcih would allow us to move to use the dependency if we wish without breaking changes. However, I will let it up to you and @estroz.
There was a problem hiding this comment.
I am not sure why the changes made make the code more understandable or not. I think it also needs to be checked by @estroz.
There was a problem hiding this comment.
HI @Adirio,
I am ok with the changes, however, I think it needs the @estroz ok as well if possible.
My concern would only : #1745 (comment)
Also, note that it is no passing in the CI. could you please fix that?
k8s-ci-robot
added
the
approved
camilamacedo86
dismissed
their stale review
I think it also needs to be checked by @estroz.
camilamacedo86
removed
the
approved
- Used non-capturing groups when the info is not relevant - Simplified some expressions Signed-off-by: Adrian Orive <[email protected]>
k8s-ci-robot
added
size/M
| @@ -47,27 +47,27 @@ var _ = Describe("Resource Options", func() { | |||
| options := &Options{Group: "crew", Version: "1", Kind: "FirstMate"} | |||
| Expect(options.Validate()).NotTo(Succeed()) | |||
| Expect(options.Validate().Error()).To(ContainSubstring( | |||
| `version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1)`)) | |||
| `version must match ^v\d+(?:alpha\d+|beta\d+)?$ (was 1)`)) | |||
There was a problem hiding this comment.
Could we not use fmt.String and have the regex as a const arg of it?
There was a problem hiding this comment.
I also thought of that to prevent having it hardcoded a dozen times the thing is that the string is not exported from its package. I think the better approach would be to create an expecific error instead a generic one and test if this yields this kind of error. But I just wanted to do the less changes to get it to pass Travis for now.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Adirio, estroz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/test pull-kubebuilder-e2e-k8s-1-16-2 |
Descrition
Motivation
While non-capturing groups should be a bit more performant that capturing groups, the improvement will be negligible. The motivations is mainly improving the clarity and readability of the regex expressions by simplifying some patters and not having unused capture groups.
/kind cleanup