HEP-FCC / heppy

[deprecated] A python analysis framework for high energy physics
Other
11 stars 32 forks source link

moving to pypi, new tools, a few bug fixes #90

Closed cbernet closed 5 years ago

cbernet commented 5 years ago

Packaging:

Physics tools:

Papas detectors:

Other tools:

cbernet commented 5 years ago

@vvolkl @JavierCVilla Here it is. Valentin, I will include your changes in all your PRs as soon as this one is merged if you agree. Javier, not sure what needs to be done for the integration tests... the heppy test suite is working, but we should make sure it is used properly in Jenkins.

vvolkl commented 5 years ago

Great! No problem, it should be easy to rebase my PR's once this is merged. I can do a review tomorrow.

cbernet commented 5 years ago

@vvolkl for your review, maybe you just want to look at the differences between master and bc354d0c, before the big restructuring for pypi. It's going to be easier for you I guess. Thanks!

JavierCVilla commented 5 years ago

This is what I meant with my comments on #89 , it is quite though to review all these changes on a single PR, even to test it. For the future, we should try to break this down into smaller PR's.

cbernet commented 5 years ago

There are not many changes apart from the pypi restructuring. If you want I can do a first PR before the pypi restructuring, and then another one?

Anyway are you sure you want to review this code? Are you a heppy user? Maybe @clementhelsens would be a good candidate.

JavierCVilla commented 5 years ago

There are not many changes apart from the pypi restructuring.

Well.. there are 1,634 lines added and 864 deleted, I think it is a considerable amount of changes.

Anyway are you sure you want to review this code?

Absolutely not...

Are you a heppy user?

.. and no, i'm not. I don't think I'm in position to review this code, but I do think that for the sake of the maintenance, we should avoid PR's of this size.

For instance, there are changes related to heppy functionality and changes related to the pypi restructuring. I guess now heppy gets installed through pip, so the CI will never work because it uses a different mechanism to build and test the package. Alternatively you can first change the CI, however in that case you are not testing the changes related to heppy functionality.

Also, once this have been merged, the new version should be directly 3.0.

Said that, these are only my two cents.

cbernet commented 5 years ago

I'm sorry Javier, but it appears you might have misunderstood my post.

I said that there are not many changes apart from the pypi restructuring and you give me the total changes in this PR including the pypi restructuring :-)

Most of the changes come from the pypi restructuring, and there is nothing we can do, this is big, even if I had done a PR specifically for that.

I understand your point about having smaller PRs and I agree. So I proposed to make 2 PRs: one for what comes before the pypi restructuring (@clementhelsens could review it), and one of the pypi restructuring. that's precisely what you asked for : smaller PRs.

So would you like me to close this PR and replace it by the two smaller ones?

JavierCVilla commented 5 years ago

The thing is that even if the vast majority of changes are related to the pypi restructuring, it is hard to distinguish. So one may need to go line by line on the review anyway.

I understand your point about having smaller PRs and I agree. So I proposed to make 2 PRs: one for what comes before the pypi restructuring, and one of the pypi restructuring. that's precisely what you asked for : smaller PRs.

So would you like me to close this PR and replace it by the two smaller ones?

IMO it would be the right way to do it.