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

user API for collapse plugin #1641

Merged
merged 2 commits into from
Sep 14, 2022
Merged

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Sep 9, 2022

Description

This pull request extends on #1401 and provides user-API access to the collapse plugin.

Plugin User API docs

TODO:

  • add PR to changelog if before next release, otherwise add new entry (waiting until PR is accepted otherwise to avoid constant merge conflicts)

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set?
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@kecnry kecnry added this to the 2.11 milestone Sep 9, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Base: 86.32% // Head: 86.33% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (35a7d10) compared to base (954f2b9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1641      +/-   ##
==========================================
+ Coverage   86.32%   86.33%   +0.01%     
==========================================
  Files          95       95              
  Lines        9607     9616       +9     
==========================================
+ Hits         8293     8302       +9     
  Misses       1314     1314              
Impacted Files Coverage Δ
...daviz/configs/default/plugins/collapse/collapse.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kecnry kecnry marked this pull request as ready for review September 9, 2022 17:11
Comment on lines +95 to +98
snackbar_message = SnackbarMessage(
f"Data set '{self.dataset_selected}' collapsed successfully.",
color="success",
sender=self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing this snackbar message either when I use the API or the UI. Do you see it trigger successfully?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think success messages are filtered to the logger window by default. Is it there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh right, I forgot we had filtered the snackbar, it's there. I wonder if it would be useful to add an indicator to the logger icon if there are unviewed messages there...

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@duytnguyendtn duytnguyendtn left a comment

Choose a reason for hiding this comment

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

Generally works well! I do feel like add_results isn't exactly clear to me though. It took me a bit to figure out that the label and auto flag were nested under add_results. I would imagine something like data_label would be more clear

And then for the viewer, I honestly feel like it ought to be in a separate element entirely. It doesn't seem connected enough to the labels to warrant having them be nested. Thoughts?

@kecnry
Copy link
Member Author

kecnry commented Sep 14, 2022

@duytnguyendtn - that affects all plugins that are able to expose results back to the app. All those pieces need to interact with each other as a single component, which is why they're built that way under-the-hood. We could expose the underlying traitlets instead, but then miss out on some of the magic and checks. Or if you have another suggestion for how to either design the underlying object or expose it differently to the user, we could consider that (but should do that before this becomes public API in any plugin).

@duytnguyendtn
Copy link
Collaborator

Ahh, good context to know! In THAT case, it's probably too little of a nitpick to justify such a big part that impacts so much. Let me do my final checks and I'll approve

Copy link
Collaborator

@duytnguyendtn duytnguyendtn left a comment

Choose a reason for hiding this comment

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

Tested in cubeviz and it worked well. Thanks!

@kecnry kecnry merged commit 255a319 into spacetelescope:main Sep 14, 2022
@kecnry kecnry deleted the api-collapse branch September 14, 2022 18:50
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.

3 participants