Registry explore cmd #92

corydickson · 2020-02-24T15:51:12Z

What was wrong?

Issue #88

How was it fixed?

Added a new subparser to registry that accepts a valid URI and prints the total releases with their corresponding version and manifest_uri as well as the package name.

Cute Animal Picture

Add tests for cli output Fix pm functions More formatting / linting

corydickson · 2020-02-24T15:52:20Z

ethpm_cli/commands/registry.py

+            pkg_version,
+            namespaced_asset,
+            ens,
+        ) = parse_registry_uri(uri)


We shouldn't need any of these other than the address but was unable to find a suitable function, I'll use underscore to remove these unused variables

parse_registry_uri(uri) returns a RegistryURI type, which is a custom NamedTuple - so you should be able to do something like

parsed_registry_uri = parse_registry_uri(uri)

and then access properties on it like

parsed_registry_uri.address

corydickson · 2020-02-24T15:53:20Z

tests/cli/test_registry_cli.py

+        f"ethpm registry explore erc1319://0x16763EaE3709e47eE6140507Ff84A61c23B0098A:1"
+    )
+    child_five.expect(ENTRY_DESCRIPTION)
+    child_five.expect("\r\n")


If you like the current formatting I'll add additional tests for an existing registry live on mainnet, hopefully something with fewer entries 😭

Actually after some thought that might be a bit flaky in the future if people add packages and the order is not preserved... maybe we should just add a new test case that releases a local package to a fresh registry

Looks good to me! That would be ideal - but I think that would require us to have the tester chain up and running? But, we get a free out of jail card here b/c of the way the registry contract is written it returns the package ids in the same order every time - new ones are at the end of the list. So, you should be safe to use one of the mainnet registries and just test that the first handful of packages are displayed.

corydickson · 2020-02-24T15:55:34Z

I'll push a fix for the linter if the formatting looks good on the output and add the corresponding tests!

njgheorghita

It's a great start, thanks! Just some changes to be made - let me know if you have any questions

njgheorghita · 2020-02-25T02:56:46Z

ethpm_cli/commands/registry.py

+            pkg_version,
+            namespaced_asset,
+            ens,
+        ) = parse_registry_uri(uri)


parse_registry_uri(uri) returns a RegistryURI type, which is a custom NamedTuple - so you should be able to do something like

parsed_registry_uri = parse_registry_uri(uri)

and then access properties on it like

parsed_registry_uri.address

njgheorghita · 2020-02-25T02:59:36Z

ethpm_cli/commands/registry.py

+            namespaced_asset,
+            ens,
+        ) = parse_registry_uri(uri)
+        config.w3.enable_unstable_package_management_api()


I'm not sure you need this line here - was it raising an error without it? The enable_unstable_package_management_api() flag should have been handled in config.py

njgheorghita · 2020-02-25T03:03:49Z

ethpm_cli/parser.py

@@ -141,6 +142,12 @@ def add_package_version_to_parser(
    )


+def add_uri_to_parser(parser: argparse.ArgumentParser, help_msg: str,) -> None:


You can remove the trailing comma after str here. Trailing commas are typically only useful when there is only one argument per line aka

things = ( parser: argparse.ArgumentParser, help_msg: str, )

njgheorghita · 2020-02-25T03:06:58Z

ethpm_cli/parser.py

+    "explore", help="Explore a registry's list of released packages and manifest uris.",
+)
+add_uri_to_parser(
+    registry_explore_parser, "Registry URI for target registry.",


Were we thinking of also supporting any aliased registries? I think it'd be more useful than requiring the entire uri everytime you want to explore a registry. imo ethpm registry explore ens > ethpm registry explore ethpm://ens.snakecharmers.eth/ for example. I think we do something similar for ethpm registry activate

Ahh okay I see the desired behavior; I'll remove the error thrown directly in explore_registry and just use resolve_uri_or_alias

njgheorghita · 2020-02-25T03:12:09Z

tests/core/test_registry.py

@@ -141,3 +142,8 @@ def test_unable_to_activate_nonexistent_aliased_registry(config):
    add_registry(URI_1, "mine", config)
    with pytest.raises(InstallError):
        activate_registry("other", config)
+
+
+def test_explore_uri_invalid_raises_error(config):


This looks like a great place to use @pytest.mark.parametrize - we're big fans of this decorator and tend to use it a lot. It lets you test a lot of varied assumptions without a ton of bloat. I'd load it up with any useful or weird edge cases you want to test.

To follow up the rest of the review would we want to even test that this function is throwing an error here anymore? Instead make it something like test_resolve_uri_or_alias_raises_error and try to hit all the cases

Yeah that's a better idea, i'd just write tests that resolve_uri_or_alias fails/passes for all the various edge cases.

njgheorghita · 2020-02-25T03:18:24Z

ethpm_cli/commands/registry.py

+        display_packages(package_names, config)
+
+    else:
+        raise UriNotSupportedError("Cannot resolve both an alias and registry uri.")


I'm not sure I understand this error message. It seems as though we currently aren't accepting an alias? Also, I tend to use f-strings a lot in my error messages - displaying the incorrect variable in the mesage can be extremely useful for users and yourself later on.
Something more like...

else: raise UriNotSupportedError(f"Invalid URI: {uri}, unable to explore. Registry URI definition can be found at https://docs.ethpm.com/uris#registry-uris.")

See the other comment below but I think it's okay to get rid of this if we are going to use existing error messages?

Yep, that looks like it should be fine

njgheorghita · 2020-02-25T03:28:08Z

ethpm_cli/commands/registry.py

+        all_releases = config.w3.pm.get_all_package_releases(package_name)
+        cli_logger.info(f"Retrieving releases for {bold_blue(package_name)}: \n")
+        for release in all_releases:
+            (version, manifest_uri) = release


I think you can do for version, manifest_uri in all_releases:

njgheorghita · 2020-02-25T03:28:18Z

ethpm_cli/commands/registry.py

+            (version, manifest_uri) = release
+            cli_logger.info(f"{bold_green(version)} --- ({bold_white(manifest_uri)})")
+        cli_logger.info(
+            f"Total: {config.w3.pm.get_release_count(package_name)}\n"


nitpick be a bit more descriptive here - total what?

njgheorghita · 2020-02-25T03:31:45Z

tests/cli/test_registry_cli.py

+        f"ethpm registry explore erc1319://0x16763EaE3709e47eE6140507Ff84A61c23B0098A:1"
+    )
+    child_five.expect(ENTRY_DESCRIPTION)
+    child_five.expect("\r\n")


Looks good to me! That would be ideal - but I think that would require us to have the tester chain up and running? But, we get a free out of jail card here b/c of the way the registry contract is written it returns the package ids in the same order every time - new ones are at the end of the list. So, you should be safe to use one of the mainnet registries and just test that the first handful of packages are displayed.

corydickson · 2020-02-28T23:59:11Z

Should have the tests ready this weekend, bit busy this week sorry for the delay

njgheorghita · 2020-03-03T04:25:33Z

@corydickson No worries! just ping me whenever this is ready for a review

corydickson · 2020-03-21T23:40:15Z

@njgheorghita Thank you for the patience, should be ready for review!

corydickson · 2020-03-22T02:09:10Z

May I make the changes suggested by the linter here?

njgheorghita

Looks good to me! Just a couple nitpicks left. Thanks for wrapping this one up - hope you're well!

njgheorghita · 2020-03-24T21:51:27Z

tests/cli/test_registry_cli.py

+    child_five.expect("\r\n")
+    child_five.expect(f"Total releases: 1")
+    child_five.expect("\r\n\r\n")
+    child_five.expect_exact(f"Retrieving all releases for {bold_blue('dai-dai')}: ")


Oh cool! Wasn't aware of expect_exact

njgheorghita · 2020-03-24T21:56:43Z

tests/core/test_registry.py

+        assert registry.alias == expected["alias"]
+        registry = resolve_uri_or_alias(uri, store_path)
+        assert registry.uri == expected["uri"]
+    else:


I'm sure I've given you conflicting rules of thumbs on how to do this, but imo - if statements are a good indicator that a test should be split up - especially if it's for valid/invalid cases. In this instance I'd split this test b/w the valid/invalid cases.

Got it, I'll split these up

njgheorghita · 2020-03-24T22:00:49Z

ethpm_cli/commands/registry.py

@@ -203,3 +216,15 @@ def generate_registry_store_data(
        "alias": alias,
        "active": activate,
    }
+
+
+def display_packages(all_package_names: Iterable[str], config: Config) -> None:


You could probably get away with just passing the w3 that exists on config here rather than the entire config instance - just to tidy things up a bit.

njgheorghita · 2020-03-24T22:03:46Z

ethpm_cli/commands/registry.py

+def display_packages(all_package_names: Iterable[str], config: Config) -> None:
+    for package_name in all_package_names:
+        all_releases = config.w3.pm.get_all_package_releases(package_name)
+        cli_logger.info(f"Retrieving all releases for {bold_blue(package_name)}: \n")


I'd put line 224 above 223 - since the call to w3 to fetch all of the releases could take a couple of seconds, it would be better to have this message displayed before that call is made.

njgheorghita · 2020-03-24T22:05:45Z

ethpm_cli/commands/registry.py

+        for version, manifest_uri in all_releases:
+            cli_logger.info(f"{bold_green(version)} --- ({bold_white(manifest_uri)})")
+        cli_logger.info(
+            f"Total releases: {config.w3.pm.get_release_count(package_name)}\n"


You should be able to get away without this w3 call and just use len(all_releases) to get the count.

njgheorghita · 2020-03-24T22:08:16Z

ethpm_cli/commands/registry.py

+        cli_logger.info(
+            f"Total releases: {config.w3.pm.get_release_count(package_name)}\n"
+        )
+    cli_logger.info(f"Packages in the registry: {config.w3.pm.get_package_count()}\n")


Any particular reason that this comes at the end of the function? It might be more useful at the beginning - like if there are 1000 packages available, it'd be better to know before all of the calls for each particular package rather than after all 1000 calls have been made and displayed.

No particular reason but your suggestion makes a lot more sense. I'm not going to add a test for this line though to prevent it from being flaky in the future

njgheorghita · 2020-03-24T23:02:25Z

Also the mypy fixes are in master now if you want to rebase against master to fix the linting errors.

corydickson · 2020-03-28T01:14:12Z

lol sorry for the fp was having issues... should be rebased correctly

njgheorghita · 2020-03-30T19:19:16Z

Looks good, thanks @corydickson !!

corydickson added 2 commits February 20, 2020 03:21

Add args for explore cmd

542dd24

WIP: Retrieve package_ids and display the output

e5622a4

Add tests for cli output Fix pm functions More formatting / linting

corydickson requested a review from njgheorghita February 24, 2020 15:51

corydickson commented Feb 24, 2020

View reviewed changes

njgheorghita requested changes Feb 25, 2020

View reviewed changes

corydickson added 4 commits February 25, 2020 20:46

WIP: Code review notes / additonal tests for errors and console output

9023efb

Create uri_or_alias argument function

d5fc8e4

Test console output for first three packages in the registry

60ea4bb

Fixed to support aliases activated in registry

ae9db72

Add tests for resolve_uri_or_alias

063985a

njgheorghita approved these changes Mar 24, 2020

View reviewed changes

njgheorghita mentioned this pull request Mar 24, 2020

Update broken mypy types #95

Merged

corydickson force-pushed the registry-explore-cmd branch 5 times, most recently from 74fd6aa to 2b660af Compare March 28, 2020 01:12

CR @njgheorghita, updated params and console output

f7c6689

corydickson force-pushed the registry-explore-cmd branch from 2b660af to f7c6689 Compare March 28, 2020 01:13

njgheorghita merged commit 432e3aa into ethpm:master Mar 30, 2020

		@@ -141,6 +142,12 @@ def add_package_version_to_parser(
		)


		def add_uri_to_parser(parser: argparse.ArgumentParser, help_msg: str,) -> None:

Registry explore cmd #92

Registry explore cmd #92

Conversation

corydickson commented Feb 24, 2020

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corydickson commented Feb 24, 2020

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corydickson commented Feb 28, 2020 • edited Loading

njgheorghita commented Mar 3, 2020

corydickson commented Mar 21, 2020

corydickson commented Mar 22, 2020

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corydickson Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

njgheorghita commented Mar 24, 2020

corydickson commented Mar 28, 2020

njgheorghita commented Mar 30, 2020

corydickson commented Feb 28, 2020 •

edited

Loading

corydickson Mar 28, 2020 •

edited

Loading