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

Refactor title decoration #373

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Jun 13, 2024

This will make it easy to reuse for decorating accordion or tab titles. The original version could only decorate an HTML widget that it had created.

#372 should be merged before this one is.

mwcraig added 3 commits June 13, 2024 08:54
With this, the _testing_path argument can be removed from saved settings
code. The alternative to going this route was to add _testing_path to
another class, the ReviewSettings widget.
This will make it easy to reuse for decorating accordion or tab titles.
The original version could only decorate an HTML widget that it had
created.
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.69%. Comparing base (318385a) to head (94858de).
Report is 218 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
+ Coverage   77.67%   77.69%   +0.02%     
==========================================
  Files          27       27              
  Lines        3628     3632       +4     
==========================================
+ Hits         2818     2822       +4     
  Misses        810      810              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JuanCab
JuanCab previously approved these changes Jun 13, 2024
Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward except for the code supporting Python 3.10 (which is fine to keep).

@@ -1,6 +1,18 @@
# Some settings require custom widgets to be displayed in the GUI. These are defined in
# this module.

# This workaround is for Python < 3.11. It is not needed in Python 3.11 and later.
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we just require 3.11? I think we opted to require 3.10 last summer, but at this point... In any case, if you are wedded to supporting 3.10, keep this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could require 3.11. I wasn't aware until I opened this PR that StrEnum was new in 3.11.

@mwcraig mwcraig dismissed JuanCab’s stale review June 14, 2024 23:55

The merge-base changed after approval.

@mwcraig mwcraig merged commit 4837f1b into feder-observatory:main Jun 14, 2024
12 checks passed
@mwcraig mwcraig deleted the refactor-title-decoration branch June 14, 2024 23:57
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.

2 participants