Skip to content
This repository was archived by the owner on Jan 13, 2023. It is now read-only.

Conversation

@lzpap
Copy link
Collaborator

@lzpap lzpap commented Dec 5, 2019

Related issue #268

Changes

Remove auto command discovery.

  • API classes could automatically discover at runtime which commands they support, and add them to their command registry. This way you could call a command on the API instance directly (api.getTrytes(..) for example). This feature turned out not to have real benefits, as a user would always call the API method, not the command directly (api.get_trytes(...)) and it also messes with the static analysis of the IDE (autocompletion, suggestion, type hints, etc...).
  • The feature was only used in tests, so corresponding test methods were modified.

Add Advanced: PyOTA Commands page to docs.

  • Explain how Commands are used within API methods.
  • Show command execution flow with visuals.
  • Explain filters, add reference to filters documentation.
  • Describe PyOTA specific filter classes.
  • Talk about difference between core and extended commands, show how one might implement new Extended API methods in PyOTA.

The Advanced nature of the doc page points out that this part is intended for people who would like to know how PyOTA works under the hood (advanced users/developers of the lib).

@lzpap lzpap self-assigned this Dec 5, 2019
Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Looks good! Added a few questions/suggestions.


:py:class:`BaseCommand` is also decorated with a :py:class:`CommandMeta`
metaclass, that helps to register each command automatically in the API classes'
command registry at runtime.
Copy link
Contributor

@todofixthis todofixthis Dec 5, 2019

Choose a reason for hiding this comment

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

💭 We could probably deprecate and remove this functionality; I don't think anybody uses the command registries.

Short version is, I wanted to make it so that developers didn't have to add methods to the Iota/StrictIota/MultisigIota/etc. classes explicitly. Instead, the registry would automatically load commands in the specified packages and attach them magically.

In exchange for being clever (i.e., no real benefits 😅), this approach had a number of drawbacks:

  • Very difficult to understand which commands a particular API class can use via static analysis.
  • Difficult to document how users should invoke each command, what parameters to provide, etc.
  • IDEs couldn't support jumping from a command invocation to the corresponding class (e.g., self.commands['getNodeInfo'] vs. core.GetNodeInfoCommand).

In #30 I laid the groundwork for making command registries redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed command registries and modified command test cases. take a look on dbb4de7 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! I added some comments to that commit.

@lzpap lzpap force-pushed the docs_advanced_pyota_commands branch 2 times, most recently from c29fafb to 6bea33f Compare December 9, 2019 16:38
@lzpap lzpap added the deprecation Removal of obsolete functionality. label Dec 9, 2019
@lzpap lzpap force-pushed the docs_advanced_pyota_commands branch from 6bea33f to a39c527 Compare December 9, 2019 17:00
@lzpap lzpap changed the title docs: add Advanced: PyOTA Commands page docs: add Advanced: PyOTA Commands page & refactoring Dec 10, 2019
@lzpap lzpap force-pushed the docs_advanced_pyota_commands branch from a39c527 to 0f2aa3b Compare December 10, 2019 12:36
- Removed auto command discovery in API class,
  as it was only used in tests.
- Modified test cases.
- Removed custom __getattr__() from API class.
Explain the inner workings of an API command for
advanced users/developers of the the library.
@lzpap lzpap force-pushed the docs_advanced_pyota_commands branch from 0f2aa3b to be39875 Compare December 10, 2019 12:40
Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Looks good!

@lzpap lzpap merged commit 585ab08 into iotaledger-archive:develop Dec 11, 2019
@lzpap lzpap mentioned this pull request Dec 11, 2019
@lzpap lzpap deleted the docs_advanced_pyota_commands branch January 28, 2020 10:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

deprecation Removal of obsolete functionality. documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants