-
Notifications
You must be signed in to change notification settings - Fork 10
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
Some refactoring of z_order fuctionality #303
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev_master #303 +/- ##
==============================================
+ Coverage 80.04% 80.07% +0.03%
==============================================
Files 145 147 +2
Lines 14771 14819 +48
==============================================
+ Hits 11823 11866 +43
- Misses 2948 2953 +5 ☔ View full report in Codecov by Sentry. |
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.
Clearly better!
After a more general discussion on the topic with @astronomyk and @hugobuddel, the effects should not be ordered according to the final two digits of their z-orders, but rather the order should follow that of definition in the respective IRDB yaml files (which is the current behavior). Ultimately, the z_order might be removed completely, so it's not worth investing much if any time in any functionality related to it.
However, that discussion came after the commits found in this PR (except the final one), and everything else can still be considered and improvement, even if it's only for the docstrings, so I'm still in favor of merging. The only test that now fails is the codecov, but creating new tests for something that will likely be removed soon anyway, seems unnecessary, so I'd just ignore that.