Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

include alias in pretty print #949

Merged
merged 6 commits into from
Jun 5, 2019

Conversation

jseagrave21
Copy link
Contributor

What current issue(s) does this address, or what feature is it adding?

How did you solve this problem?

  • if an alias exists for the address, print the alias
  • updated tests

NOTE: Right now there is no ability within neo-python to update/delete the alias once it is set. I think this should be a future feature update.

How did you make sure your solution works?
manual testing initially and make test after tests were updated

Are there any special changes in the code that we should be aware of?
no

Please check the following, if applicable:

  • Did you add any tests? (updated)
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • Did you add an entry to CHANGELOG.rst? (if not, please do)

jseagrave21 added 2 commits May 29, 2019 23:07
- if an alias exists for the address, print the alias
@coveralls
Copy link

coveralls commented May 30, 2019

Coverage Status

Coverage increased (+0.01%) to 85.122% when pulling 6dd815a on jseagrave21:pretty-print-alias-2 into 475402d on CityOfZion:development.

@ixje
Copy link
Member

ixje commented May 30, 2019

I could not resolve the conflict via Github web. For some reason it won't load completely so I can't update the CHANGELOG. You'll have to do it 🤷‍♂

@jseagrave21
Copy link
Contributor Author

@ixje okay, it should be good now

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

if you can also resolve the readme on this branch then I can merge. The webbased "resolve conflicts" isn't working for this one either (probably due to same branching point 🤷‍♂ )

@@ -525,8 +525,17 @@ def pretty_print(self, verbose=False):
'tokens': self._get_token_balances(addr_str)
}})

aliases = dict()
alia = NamedAddress.select()
if len(alia):
Copy link
Member

Choose a reason for hiding this comment

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

we can skip this and do for n in NamedAddress.select() as the return value is always a ModelSelect type which supports iterating

# pretty print
for address, data in addresses.items():
for title, addr in aliases.items():
Copy link
Member

Choose a reason for hiding this comment

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

Just as a general comment;

I think we could have used NamedAddress.get() to query if an alias exists for the given address/scripthash instead of iterating over all available data. Now it doesn't really matter as we won't have a huge list of aliases. Just something to try and keep in mind when you encounter some performance critical part with larger data sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback!

@ixje ixje merged commit d61452a into CityOfZion:development Jun 5, 2019
@ixje
Copy link
Member

ixje commented Jun 5, 2019

I changed the log entry to

Include address aliases in ``wallet`` command output

because your entry described the internal change that requires "inside" knowledge. This describes the impact for the user

@jseagrave21
Copy link
Contributor Author

I changed the log entry to

Include address aliases in ``wallet`` command output

because your entry described the internal change that requires "inside" knowledge. This describes the impact for the user

Thank you! I will try to keep that in mind for future Changelog entries.

@jseagrave21 jseagrave21 deleted the pretty-print-alias-2 branch June 5, 2019 11:43
@ixje
Copy link
Member

ixje commented Jun 12, 2019

50 points

@ixje ixje added this to Pending Distribution in Awards Distribution Jun 12, 2019
@lllwvlvwlll lllwvlvwlll moved this from Pending Distribution to In Progress in Awards Distribution Jul 17, 2019
@lllwvlvwlll
Copy link
Member

lllwvlvwlll commented Jul 18, 2019

@lllwvlvwlll lllwvlvwlll moved this from In Progress to Distributed in Awards Distribution Jul 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants