-
Notifications
You must be signed in to change notification settings - Fork 3
Mastodon reader + label changing #84
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
Mastodon reader + label changing #84
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## code-split-2 #84 +/- ##
=================================================
- Coverage 96.77% 63.24% -33.53%
=================================================
Files 2 15 +13
Lines 341 2487 +2146
Branches 17 0 -17
=================================================
+ Hits 330 1573 +1243
- Misses 6 914 +908
+ Partials 5 0 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -763,16 +763,17 @@ def read_from_tgmm_xml( | |||
|
|
|||
|
|
|||
| def read_from_mastodon( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If understand this correctly it only reads one label set, right? If yes maybe we can read every label?
| def available_labels(lT: LineageTree) -> list[str]: | ||
| """Returns the list all the available label dictionaries | ||
|
|
||
| Returns | ||
| ------- | ||
| list of string | ||
| list of the names of all the available properties | ||
| to label the nodes | ||
| """ | ||
| available_labels = [] | ||
| for prop_name, prop in lT.__dict__.items(): | ||
| if ( | ||
| 0 < len(prop_name) | ||
| and prop_name[0] != "_" | ||
| and isinstance(prop, dict) | ||
| and 0 < len(prop) | ||
| and all(isinstance(k, int) for k in prop.keys()) | ||
| and all(isinstance(v, str) for v in prop.values()) | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See next comment. I think we should have a specifc class for the labels, called available labels or something.
| .. warning:: The labels are changed in place. | ||
| Therefore, the former labels dictionary was not | ||
| saved it is lost. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my previous comment, I think that you should have all the labels in one place, and then change them whenever and when you change labels you should save the old ones (so you can use different labelling systems). A solution like this could be far easier because:
lT.labels = available_labels[i] to do this we should make a special class so that we only accept assignment of labels if it comes from a specific class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I fully understand your suggestion yet.
All the labels are already in one place (LineageTree) granted they are with other things but still, they are all in one place.
When you write lT.labels = available_labels[i] do you mean the user would do that?
If yes how would he build available_labels, if he doesn't build it, where would he find it?
How would he modify it?
How that would be attached to a specific instance of LineageTree?
If the user can modify it, how would we make sure that they do it correctly?
There are too many unkown to understand your suggestion.
Also, the .. warning:: is not relevant anymore, we keep the labels after change.
…ge_label; renaming `available_labels` to `get_available_labels`
In this: