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

Add warning about floating-point arithmetic to foreach introduction #1053

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

3geek14
Copy link
Contributor

@3geek14 3geek14 commented Sep 20, 2021

Signed-off-by: 3geek14 [email protected]

Motivation for this change

On #1050, @muzimuzhi suggested it might be worth adding a warning about floating-point errors in foreach loops to the tutorial where they are first introduced. I think my suggested change might be a bit cryptic about what sorts of rounding errors show up, but I don't know how explicit it's worth being here, when it's spelled out in far more detail in section 88, Repeating Things: The Foreach Statement. I also added to the example code a second loop that does have a rounding error preventing the final tick from being drawn.

Checklist

Please check the boxes below and signoff your commits to explicitly
state your agreement to the Developer Certificate of Origin:

  • [ ✓ ] Code changes are licensed under GPLv2 + LPPLv1.3c
  • [ ✓ ] Documentation changes are licensed under FDLv1.2

@hmenke
Copy link
Member

hmenke commented Sep 20, 2021

Could you please write a little more succinct commit messages? Commit messages can have more than one line, so something like this would be more appropriate:

manual: add warning about floating-point arithmetic #1050

Per a suggestion from @muzimuzhi, add a warning about floating-point arithmetic
to the first introduction of `foreach` in Karl's tutorial.

See also https://www.conventionalcommits.org/, although we do not entirely practice this here (yet).

@3geek14
Copy link
Contributor Author

3geek14 commented Sep 20, 2021

Thanks for the link, I'll try to write better commit messages in the future (as well as try to remember to sign my commits). Since I'm not sure how to retroactively sign commits, my plan was to close this PR and submit a new one. Should I instead fix this one somehow?

@hmenke
Copy link
Member

hmenke commented Sep 20, 2021

You can run git rebase --signoff -i origin/master and replace all pick by reword to rewrite the commit messages and automatically signoff.

@3geek14 3geek14 force-pushed the add-floating-point-warning branch from 0408293 to 8b02a24 Compare September 20, 2021 18:40
Per a suggestion from @muzimuzhi on pgf-tikz#1050, add a warning about floating-point arithmetic
to the first introduction of `foreach` in Karl's tutorial.

Signed-off-by: 3geek14 <[email protected]>
@muzimuzhi
Copy link
Member

Hmm applying suggested changes on github's webpage (by adding "Co-authored-by" trailer) will always not add the "Signed-off-by" trailer, which makes the check failed. Even worse when the two trailers are used together, whom the "I" used in Developer Certificate of Origin is referring would be not unique / unclear. Someone may interpret that only the committer is signed off, not including the one co-authored.

@hmenke What's your opinion in the long term?

@hmenke
Copy link
Member

hmenke commented Sep 25, 2021

Signed-off-by is not really important for small fixes. It's more important for new features. I don't want to get trolled again by someone who doesn't understand their own licensing terms.

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

Successfully merging this pull request may close these issues.

3 participants