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

Cosmetic changes from #1737 #1740

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Cosmetic changes from #1737 #1740

merged 1 commit into from
Apr 22, 2020

Conversation

JustinDrake
Copy link
Collaborator

Pulling out some of the cosmetic changes from #1737:

  • Rename the SigningRoot container to SigningData. The signing_root (of type Root) is the hash_tree_root of the SigningData (of type Container).
  • Cleanup compute_signing_root
  • Remove unnecessary new lines.

(The rationale for this split is because the substantive changes from #1737 may not make it in phase 0. Also this will help focus the discussion in #1737 on the substantive changes.)

@JustinDrake JustinDrake marked this pull request as ready for review April 21, 2020 17:00
@hwwhww hwwhww changed the base branch from dev to v012x April 22, 2020 03:38
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the target branch to v012x(dev branch for the backward compatible changes)

I know devs hate renamings but this PR makes sense. LGTM 👍

@djrtwo
Copy link
Contributor

djrtwo commented Apr 22, 2020

I really think we should avoid nitpicking the spec at this point. Any non-security critical changes we introduce add unnecessary overhead for client teams and the potential to introduce minor bugs.

I agree that this looks good and the PR is a very small and digestible. Okay to merge.
But, I reiterate, now is generally not the time for a wave of renamings and code restructurings even if non-substantive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants