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

Ensure tags are copied and presented when moving Collections #4277

Merged
merged 6 commits into from Jul 14, 2017

Conversation

Projects
None yet
5 participants
@pvanheus
Copy link
Contributor

commented Jul 5, 2017

This addresses #4249. There were 2 problems:

  1. The tags were not copied by the collections manager.
  2. When converting to a dict for presentation to the UI, the tags were not added to the dict.

@galaxybot galaxybot added the triage label Jul 5, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jul 5, 2017

@pvanheus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

Changed the way tags are copied which fixes bug with earlier patch. Note: this code needs a test.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

@galaxybot test this

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

@galaxybot test this

1 similar comment
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

@galaxybot test this

@@ -1157,7 +1177,7 @@ def is_hda(d):
return isinstance( d, HistoryDatasetAssociation )


class History( object, Dictifiable, UsesAnnotations, HasName ):
class History( HasTags, Dictifiable, UsesAnnotations, HasName ):

This comment has been minimized.

Copy link
@dannon

dannon Jul 6, 2017

Member

It works the same way, but I prefer the more explicit object, if we could leave that here. HasTags must come before Dictifiable right? MRO mess is why we may have pulled back from some of these mixins, IIRC.

This comment has been minimized.

Copy link
@pvanheus

pvanheus Jul 6, 2017

Author Contributor

The reason it was done this was is because HasTags inherits from object and calls super() in order to let Dictifiable create the original dictionary (to which it adds the 'tags' key). I changed it in my latest commit to not inherit from object and use super() so will see if that breaks anything. Specifically I'm concerned about the

This stuff certainly isn't straightforward to navigate in Python. If I understand it correctly, there should be a path from the subclass to_dict() up through the inheritance chain so that the last thing mentioned in the list is the one that actually creates the dictionary, everything else before it uses super() to get a dictionary and then adds to that dict.

My latest change has broken that principle so it might have broken the chain, since now HasTags and Dictifiable both have a to_dict() method that creates a dictionary. Not sure if the testing will catch this.

This comment has been minimized.

Copy link
@pvanheus

pvanheus Jul 6, 2017

Author Contributor

I reverted back to the HasTags inherits from object way of doing things and then removed object from the inheritance list. Looks like the only way to tame the MRO beast.

This comment has been minimized.

Copy link
@dannon

dannon Jul 6, 2017

Member

Hrmm. Yeah, my concern is that that approach isn't scalable for mixins. I'll tinker with a few things and see what I can come up with.

This comment has been minimized.

Copy link
@pvanheus

pvanheus Jul 6, 2017

Author Contributor

Sure. In the meantime 37cb522 is the simple solution for the bug this addresses.

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

@pvanheus Thanks for the changes; I thought about it more and even if this mixin isn't the long term solution (and I don't currently have anything concrete and better), it's a nice step towards consolidating functionality into a single code path that's more easily updated in the future, along with being an actual fix for collection tag copying. Merging this as-is, and we can revisit the mixin structure later, thanks again!

@dannon dannon merged commit 0f2fe60 into galaxyproject:dev Jul 14, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.