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.

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

reference: ParseNamed updated to enforce canonical format #2142

Merged

Conversation

dmcgowan
Copy link
Collaborator

Updates the ParseNamed function to require that the input be in canonical form. Throws an error if the input is not canonical. This protects against using ParseNamed to create partial Named values. These partial values can still be created explicitly through WithName and Parse, as used now used by the registry server.

In the future, the registry should start using fully canonical values on the backend and WithName should no longer support creating partial values.

@dmcgowan dmcgowan changed the title ParseNamed updated to enforce canonical format reference: ParseNamed updated to enforce canonical format Jan 14, 2017
@aaronlehmann
Copy link
Contributor

LGTM

@codecov-io
Copy link

codecov-io commented Jan 14, 2017

Current coverage is 51.21% (diff: 75.00%)

Merging #2142 into master will decrease coverage by 10.00%

@@             master      #2142   diff @@
==========================================
  Files           125        125           
  Lines         11448      11435     -13   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits           7008       5856   -1152   
- Misses         3552       4833   +1281   
+ Partials        888        746    -142   

Powered by Codecov. Last update 0111f1e...d8fcbee

@aaronlehmann
Copy link
Contributor

Needs a rebase.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
The registry uses partial Named values which the named parsers
no longer support. To allow the registry service to continue
to operate without canonicalization, switch to use WithName.
In the future, the registry should start using fully canonical
values on the backend and WithName should no longer support
creating partial values.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@dmcgowan dmcgowan force-pushed the reference-enforce-canonical-parsing branch from 08808ce to d8fcbee Compare January 18, 2017 21:57
@dmcgowan
Copy link
Collaborator Author

Rebased and tests passing, this change should get a review from @stevvooe though

@stevvooe
Copy link
Collaborator

@dmcgowan Should WithName not be a combinator?

@dmcgowan
Copy link
Collaborator Author

@stevvooe wdyt? Not returning an error so it could be combined with other methods WithTag(WithName("repo"), "tag")?

@stevvooe
Copy link
Collaborator

@dmcgowan That's not exactly what I mean. Usually, WithXXX takes an existing things and modifies. In this case, it is creating the first thing.

@dmcgowan
Copy link
Collaborator Author

@stevvooe understood. I would like to see WithName go away because it doesn't have a good fit with the other functions and "nothing with a name" ends up being a named is not straight forward. We only need this function right now as mechanism for the registry to be able to create a raw named value without normalization. Since the goal of this effort is not just to unify the downstream fork, but as to disambiguate how the package is used, it seems appropriate to rename the function. Some possible names or themes of how it could be replaced are CreateRawNamed, CreateUnnormalizedNamed. I would want to avoid "ToNamed" or something that signifies transferring a raw string to a named because it may get unintentionally used where a fully normalized name is. What I want to do in the long term is update the registry to just use a fully normalized named. I think given where we want to go, CreateNamed(domain, path string) (Named, error) could be the best fit, it would be hard to misuse and the registry could explicitly pass in the domain as "".

@stevvooe
Copy link
Collaborator

@dmcgowan This package seems like it is experiencing some serious bloat. I am not sure if I have the full context to add anything here, but CreateUnnormalizedNamed is a mouthful.

LGTM

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.

None yet

4 participants