-
Notifications
You must be signed in to change notification settings - Fork 100
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
Customisable measure data domain style #885
Customisable measure data domain style #885
Conversation
…tomisable-measure-data-domain-style
…e-measure-data-domain-style
…e-measure-data-domain-style
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #885 +/- ##
==========================================
+ Coverage 88.50% 88.53% +0.03%
==========================================
Files 172 172
Lines 20121 20107 -14
==========================================
- Hits 17808 17802 -6
+ Misses 2313 2305 -8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @ericneiva ! Thanks for the PR. I agree the API is much cleaner now. My only concern is that if we accept the PR as it is, then we should also do major release, correct? Can we may be add the old signatures and add corresponding deprecated warnings so that we avoid a major release? Also note that, since this PR (my fault sorry), we also added another kind of measure, called |
Forget this. I now see you are already taking into account |
Also dont forget to add the corresponding entry in NEWS.md ... Thanks! |
Hi, @amartinhuertas,
Good to know :)
Done. Let me know if the deprecated warnings are ok.
Done. Thank you very much for your review! |
FYI, just in case you did not know, I found this article on deprecation quite useful: https://invenia.github.io/blog/2022/06/17/deprecating-in-julia/ All good now. Accepting PR! |
Great, thanks!
Useful, indeed, thanks for sharing, @amartinhuertas |
Solves #877.
After discussion with @santiagobadia, I have changed the
CellQuadrature
constructors such that now both data and integration domain styles are keyword arguments that the user can customise.Note that this PR introduces a minor change in the API.