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

Get Repository Name #131

Merged
merged 5 commits into from
Jun 7, 2022
Merged

Get Repository Name #131

merged 5 commits into from
Jun 7, 2022

Conversation

aj-foster
Copy link
Contributor

@aj-foster aj-foster commented Feb 10, 2022

This PR is related to hexpm/hex#931.

In order to enable the modification of hex repositories without the need for a --name flag, this change adds hex_registry:get_repository_name/2.

After further discussion, this PR makes a breaking change to the return type of several registry-reading functions to return additional information that may be useful in the future.

@wojtekmach
Copy link
Member

Thank you for the PR! Let's hold off on it for a moment given this comment https://github.com/hexpm/hex/pull/931/files#r803454950

@aj-foster
Copy link
Contributor Author

@wojtekmach Not sure if you wanted to go ahead and include repository as a top-level key; just pushed a commit that does so. Easy to change if you'd prefer to keep only packages for now.

@wojtekmach
Copy link
Member

@aj-foster perfect, thank you! Could you do the same thing for functions for other registry files, that is decode_versions and decode_package?

@aj-foster
Copy link
Contributor Author

Sure thing. I went ahead and had each decoder return all of the top-level keys from each file (repository from versions; repository and name from packages).

@wojtekmach wojtekmach requested a review from ericmj June 3, 2022 08:49
Comment on lines 52 to 53
#{repository := Repository, packages := Packages} = hex_pb_names:decode_msg(Payload, 'Names'),
{ok, #{repository => Repository, packages => Packages}};
Copy link
Member

@wojtekmach wojtekmach Jun 3, 2022

Choose a reason for hiding this comment

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

hmm, actually should we just go with this?

Suggested change
#{repository := Repository, packages := Packages} = hex_pb_names:decode_msg(Payload, 'Names'),
{ok, #{repository => Repository, packages => Packages}};
{ok, hex_pb_names:decode_msg(Payload, 'Names')}.

as long as it's safe, i.e. decode_msg doesn't return an error tuple or something?

in hindsight, it was a big mistake to initially return just the subset of the protobuf, just the packages field. With this change we're in sync with the protobuf which means if we ever add another field, we'll immediately start returning it here.

Copy link
Contributor Author

@aj-foster aj-foster Jun 6, 2022

Choose a reason for hiding this comment

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

Went ahead and made this change in 85e5964 — easy to revert if necessary. I also made the similar change to decode_versions/2 and decode_package/3.

Edit: there was a choice to make for the "verified" function clause, which could use review. I have it verifying the same fields for now.

@wojtekmach
Copy link
Member

Thank you! Seems there are some transient issues on CI so I'll merge this afterwards.

@wojtekmach wojtekmach merged commit 9f3a3c8 into hexpm:main Jun 7, 2022
@wojtekmach
Copy link
Member

Thank you!

@aj-foster aj-foster deleted the aj/feat/get-repo-name branch June 7, 2022 15:20
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