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

Retrieving error types from the metadata of the Substrate palette "SubtensorModule" for the btcli console (logic) #1862

Merged
merged 3 commits into from
May 13, 2024

Conversation

roman-opentensor
Copy link
Contributor

@roman-opentensor roman-opentensor commented May 9, 2024

I decided to split #1859 into 2 PRs:

  1. Implementation of all logic (current PR)
  2. Update of existing errors with the new logic (separate PR)

Also, I had to rename the subtensor.py module due to a conflict between the class name and the module name in the namespace. This will not affect users and will not impact their experience.

@roman-opentensor roman-opentensor self-assigned this May 9, 2024
@roman-opentensor roman-opentensor added feature new feature added bittensor labels May 9, 2024
@roman-opentensor roman-opentensor force-pushed the feature/roman/retrieving-error-types branch 5 times, most recently from 2b86049 to a31da79 Compare May 10, 2024 02:49
…btensorModule" for the btcli console (logic)
@roman-opentensor roman-opentensor force-pushed the feature/roman/retrieving-error-types branch from a31da79 to ba4a700 Compare May 10, 2024 03:03
@roman-opentensor roman-opentensor marked this pull request as ready for review May 10, 2024 04:00
Copy link
Contributor

@thewhaleking thewhaleking left a comment

Choose a reason for hiding this comment

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

Hey man, this is a massive breaking change (changing subtensor.py => subtensor_module.py). Can you elaborate on the namespace conflicts that necessitate this?

@roman-opentensor
Copy link
Contributor Author

Hey man, this is a massive breaking change (changing subtensor.py => subtensor_module.py). Can you elaborate on the namespace conflicts that necessitate this?

Hey man!
Thank you for your comment and attention to the changes in the pull request. I'd like to delve into the reasons that necessitated the renaming of the subtensor.py module to subtensor_module.py.

  1. Namespace Conflict: The issue of namespace conflict between the subtensor class and the module itself was quite apparent. This created complications when directly importing the class from the module, which contradicts Python’s naming conventions where class names should start with a capital letter, unlike modules. Renaming the class was not considered feasible due to existing constraints in our codebase.
  2. Module and Class Clarity: Changing the module name eliminates confusion and makes the import structure clearer and more obvious, enhancing code readability and reducing the likelihood of import errors.
  3. Impact Analysis: Before making this change, I conducted a thorough analysis to ensure that the renaming would only affect a limited number of places in the project, allowing for easy updates. Most interactions with the subtensor class within the project occur through the project root, and these references were not affected by the module renaming.
  4. Maintenance and Scalability: This change also aims to improve the scalability and maintainability of our codebase, aligning with Python best practices for package and module naming. This simplifies the project structure understanding for future developers.

I hope these explanations help clarify the rationale behind the changes. I am open to further discussion and am willing to consider any other suggestions you might have for effectively addressing this issue.

@roman-opentensor roman-opentensor force-pushed the feature/roman/retrieving-error-types branch 2 times, most recently from e3a36d3 to ff9a752 Compare May 10, 2024 23:01
…ort subtensor module as subtensor_module on bittensor package level
@roman-opentensor roman-opentensor force-pushed the feature/roman/retrieving-error-types branch from ff9a752 to 6979acf Compare May 10, 2024 23:12
Copy link
Collaborator

@garrett-opentensor garrett-opentensor left a comment

Choose a reason for hiding this comment

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

lgtm

@roman-opentensor roman-opentensor merged commit 6230b3f into staging May 13, 2024
12 checks passed
@gus-opentensor gus-opentensor mentioned this pull request May 17, 2024
@roman-opentensor roman-opentensor deleted the feature/roman/retrieving-error-types branch May 20, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants