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

[PPS] Removed all 'get' prefixes from PPS DataFormat objects #28252

Merged
merged 21 commits into from Oct 30, 2019

Conversation

forthommel
Copy link
Contributor

PR description:

This purely technical/aesthetic PR addresses the proposal in #25690 to drop all get prefixes in PPS data formats accessors.

PR validation:

Compiles fine, backward-compatible with previously produced samples.

if this PR is a backport please specify the original PR:

Before submitting your pull requests, make sure you followed this checklist:

cc: @jan-kaspar @fabferro

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28252/12406

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@civanch
Copy link
Contributor

civanch commented Oct 28, 2019

+1

@forthommel
Copy link
Contributor Author

@perrotta, @slava77
A long and tedious 1-to-1 comparison of most of the meaningful variables (i.e. all variables propagated all along several steps of the PPS reconstruction chain, and a few isolated variables) between reference (here) and this modified release (here) for workflows 136.85, 136.731, and 136.788 shows no difference.

@perrotta
Copy link
Contributor

@perrotta, @slava77
A long and tedious 1-to-1 comparison of most of the meaningful variables (i.e. all variables propagated all along several steps of the PPS reconstruction chain, and a few isolated variables) between reference (here) and this modified release (here) for workflows 136.85, 136.731, and 136.788 shows no difference.

Thank you @forthommel

@perrotta
Copy link
Contributor

+1

  • All changes in code consist in the renaming of the getter methods, as well as a few member variables, as planned
  • Jenkins tests pass and show no differences
    _ No differences observed also in a "long and tedious 1-to-1 comparison of most of the meaningful variables" by Laurent, further certifies that there were no typing errors in the translation.

@ggovi
Copy link
Contributor

ggovi commented Oct 28, 2019

+1

@fabiocos
Copy link
Contributor

@christopheralanwest @tlampen @pohsun could you please check this PR? This should better enter in parallel with cms-sw/cms-bot#1217 to get a clean comparison

@tlampen
Copy link
Contributor

tlampen commented Oct 29, 2019

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@forthommel
Copy link
Contributor Author

@fabiocos , all,
Now that cms-sw/cms-bot#1217 is merged, may we expect this PR to be merged soon-ish? As further developments are now relying on this updated list of accessors.
Many thanks !

@fabiocos
Copy link
Contributor

@forthommel it is in my list of PRs to be merged tonight

@forthommel
Copy link
Contributor Author

@fabiocos Many thanks for the heads up! And sorry for my (usual...) insistence!

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b3526ab into cms-sw:master Oct 30, 2019
@forthommel forthommel deleted the pps-no_get-11_0_X branch October 30, 2019 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet