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

[ethPM] Update registry uri #1571

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Jan 29, 2020

What was wrong?

Updated the Registry URI utils to support URIs with ethpm as the scheme and package ids conforming to pkg_name@version - containing escaped characters

re: #1486

Todo:

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the update-registry-uri branch 2 times, most recently from 8f9e32b to a85fb39 Compare January 29, 2020 13:36
@njgheorghita njgheorghita changed the title Update registry uri [ethPM] Update registry uri Jan 29, 2020
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Any documentation updates that should go with this?

return None, None

if "@" not in pkg_id:
return pkg_id, None
Copy link
Member

Choose a reason for hiding this comment

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

This return and the one above this violate the type signagure of this function.... looks like it should be Tuple[Optional[str], Optional[str]]


if "@" not in pkg_id:
return pkg_id, None
pkg_name, safe_pkg_version = pkg_id.split("@")
Copy link
Member

Choose a reason for hiding this comment

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

This line would still be executed if pkg_id = a@b@c`.

Since version is technically allowed to be any string (i think) then I think you need to do this with pkg_id.partition('@') which might result in a safe_pkg_version with multiple @ symbols in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is covered in validate_registry_uri - which will raise an exception if there is more than 1 @ symbol - since at this stage all @ symbols should be properly escaped (i.e. "safe"). There's test cases covering both ...

  • invalid paths: pkg@1@0 (version= 1@0)
  • valid paths: pkg@1%400 (version= 1@0)

@njgheorghita
Copy link
Contributor Author

njgheorghita commented Jan 30, 2020

@pipermerriam I made one minor update: making the chain_id optional - if it's not included, the default is the mainnet. This goes one step further to making registry uris easy to use - and since we want to encourage registries on the mainnet, it seems like a safe call.

previous simplest uri: ethpm://packages.zeppelin.eth:1/[email protected]
new simplest uri: ethpm://packages.zeppelin.eth/[email protected]

Any documentation updates that should go with this?

Yup doc updates are in #1486 and I've already made the updates to docs.ethpm.com which should cover the bases

@njgheorghita njgheorghita merged commit aa18fb8 into ethereum:master Jan 30, 2020
@njgheorghita njgheorghita deleted the update-registry-uri branch January 30, 2020 16:54
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.

2 participants