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

Rename 'node' to 'container' and shorten URLs. #478

Merged
merged 7 commits into from
Jul 5, 2023

Conversation

danielballan
Copy link
Member

Closes #472.

To be determined: Should we update the client module and class tiled.client.node.Node to tiled.client.container.Container? I lean "yes" but I do not feel 100% sure. It is technically still a Node, but it is specifically a Container node, so it should be updated, right?

@danielballan
Copy link
Member Author

This also seemed like the right time to rename the queries on specs and structure families to SpecQuery and StructureFamilyQuery, to avoid confusion with the Spec dataclass and the StructureFamily enum. They are in different modules, of course, so they don't technically collide but this seemed likely to be a source of confusion for users.

@padraic-shafer
Copy link
Contributor

To be determined: Should we update the client module and class tiled.client.node.Node to tiled.client.container.Container?

Is there an existing or likely usage where you could meaningfully replace def do_something_with(node: Node): … with e.g. def do_something_with(node: DataFrame): … or def do_something_with(node: Array): …?

If not, then you can comfortably replace …Node with …Container.

@danielballan
Copy link
Member Author

Good point. There are things one can do on a Container that one cannot do on just any generic Node. We should rename so that the typing makes sense. 👍

@padraic-shafer
Copy link
Contributor

I suppose a nice way to transition the node module and its classes would be to create aliases (Node = Container) and a module-level deprecation warning.

@danielballan
Copy link
Member Author

Done!

To make this change fully backward-compatible would get messy very quickly. At this early stage, I think it's better to avoid loading up the codebase with that mess. I am opting to make only a basic effort here---raising clear error message / warnings at least.

There is a corresponding update in Databroker, ready to go once Tiled is released.

@danielballan danielballan merged commit 2c4aac7 into bluesky:main Jul 5, 2023
7 checks passed
dylanmcreynolds pushed a commit to dylanmcreynolds/tiled that referenced this pull request Jul 10, 2023
* Rename structure family 'node' to 'container', shorten URLs.

* Update tree util.

* Make test of inlined HFD5 content more thorough.

* Update URLs.

* Satisfy linters.

* Update renamed queries in docs.

* Rename client node module and Node class.
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.

Give a more distinct name to the structure family currently called "node"
2 participants