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

Better basf2-related docs, examples and change how basf2 release hash is calculated for local releases. #193

Merged
merged 15 commits into from
Apr 3, 2023
Merged

Better basf2-related docs, examples and change how basf2 release hash is calculated for local releases. #193

merged 15 commits into from
Apr 3, 2023

Conversation

GiacomoXT
Copy link
Contributor

Top-level import ROOT is very nasty and it must be avoided as much as possible. So, I removed them.

Moreover, better not to invoke the Environment singleton, but to directly use the parmaeter in basf2.process for the maximum number of events.

And in case it's not evident, I don't like basf2.create_path(). :D

@github-actions github-actions bot added the needs changelog Entry to CHANGELOG.md is missing label Mar 31, 2023
@meliache
Copy link
Collaborator

meliache commented Mar 31, 2023

Looks good to me 👍 . I assume you tested the new examples work, right?

And in case it's not evident, I don't like basf2.create_path(). :D

path = basf2.Path() is definitely more pythonic and I'm in favor of using this, calling the __init__ via the name of the type to instantiate the object instead of a separate factory method. Just a question out of curiosity, was the possibility to call basf2.Path added in some specific basf2 release or was it always there? Around five years ago when I started at Belle II everyone in my group seemed to use basf2.create_path and it seemed widespread in examples, so I'm wondering if it was introduced in some point.

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Patch coverage: 52.77% and project coverage change: +1.32 🎉

Comparison is base (1ff3782) 59.20% compared to head (374944c) 60.52%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   59.20%   60.52%   +1.32%     
==========================================
  Files          23       23              
  Lines        1554     1586      +32     
==========================================
+ Hits          920      960      +40     
+ Misses        634      626       -8     
Impacted Files Coverage Δ
b2luigi/basf2_helper/tasks.py 38.46% <0.00%> (+0.96%) ⬆️
b2luigi/batch/processes/gbasf2.py 49.78% <44.54%> (+3.54%) ⬆️
b2luigi/basf2_helper/utils.py 78.57% <75.00%> (-1.43%) ⬇️
b2luigi/core/utils.py 73.60% <88.88%> (+0.44%) ⬆️
b2luigi/__init__.py 88.46% <100.00%> (ø)
b2luigi/core/parameter.py 92.00% <100.00%> (+3.11%) ⬆️

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@GiacomoXT
Copy link
Contributor Author

path = basf2.Path() is definitely more pythonic and I'm in favor of using this, calling the __init__ via the name of the type to instantiate the object instead of a separate factory method. Just a question out of curiosity, was the possibility to call basf2.Path added in some specific basf2 release or was it always there? Around five years ago when I started at Belle II everyone in my group seemed to use basf2.create_path and it seemed widespread in examples, so I'm wondering if it was introduced in some point.

basf2.Path() is available thanks to https://gitlab.desy.de/belle2/software/basf2/-/blob/main/framework/core/src/Path.cc#L243 and https://gitlab.desy.de/belle2/software/basf2/-/blob/main/framework/scripts/basf2/core.py#L31 and it's there since many years. I think that basf2.create_path() was introduced at the beginning when the python-binding was rudimentary (it is in my to-do list for being removed since unnecessary).

@meliache
Copy link
Collaborator

meliache commented Apr 1, 2023

By the way, this PR still needs an entry to the CHANGELOG.md. I can do that and if you don't mind add you to the contributors list.

basf2.create_path() was introduced at the beginning when the python-binding was rudimentary (it is in my to-do list for being removed since unnecessary).

Sounds sensible to me. In Python it's common to first show a DeprecationWarning in one release before deprecating it in the next major release, I would suggest doing something like that.

@meliache meliache changed the title Better basf2-related documentation Better basf2-related docs, examples and change how basf2 release hash is calculated for local releases. Apr 3, 2023
@github-actions github-actions bot removed the needs changelog Entry to CHANGELOG.md is missing label Apr 3, 2023
@meliache meliache self-assigned this Apr 3, 2023
@meliache meliache added bug Something isn't working enhancement New feature or request documentation Documentation improvements labels Apr 3, 2023
meliache added 4 commits April 3, 2023 12:24
Just raising errors breaks unit tests and is just a backwards incompatible
behaviour. Changing it should be a separate PR, then unittests could be fixed by mocking.

Also added more documentation.
Still didn't test the cases for local development version, where basf2 needs to be imported
@meliache meliache merged commit 705174c into nils-braun:main Apr 3, 2023
meliache added a commit that referenced this pull request Apr 3, 2023
@GiacomoXT
Copy link
Contributor Author

@meliache thanks a lot for having taken care of the last remaining issues (I was on vacation during the last two days).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Documentation improvements enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants