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.

Sign up for GitHub

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

Jump to bottom

ATragMX - Allow one decimal place for the bullet mass #6041

Merged
merged 1 commit into from
Jan 10, 2018
Merged

ATragMX - Allow one decimal place for the bullet mass #6041

merged 1 commit into from
Jan 10, 2018

Conversation

ulteq
Copy link
Contributor

@ulteq ulteq commented Jan 10, 2018 •
edited
Loading

The bullet mass (in grains) is still going to be limited to integers.

Closes: #6040

@ulteq ulteq added the kind/enhancement Release Notes: **IMPROVED:** label Jan 10, 2018
@ulteq ulteq added this to the 3.13.0 milestone Jan 10, 2018
@ulteq ulteq changed the title ATagMX - Allow one decimal place for the bullet mass (in grams) ATagMX - Allow one decimal place for the bullet mass Jan 10, 2018
kymckay
kymckay requested changes Jan 10, 2018
View reviewed changes
Copy link
Member

@kymckay kymckay left a comment

Choose a reason for hiding this comment

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

Please use toFixed 😆

@bux bux changed the title ATagMX - Allow one decimal place for the bullet mass ATragMX - Allow one decimal place for the bullet mass Jan 10, 2018
@ulteq
Copy link
Contributor Author

ulteq commented Jan 10, 2018

There are more than 60 other places in the code where I would also have to use toFixed ...

Can we do that in a separate pull request please?

@kymckay
Copy link
Member

kymckay commented Jan 10, 2018

I'm not sure I understand the objection, why do it in a separate pull request when we could just do it in this one? 😕

It seems a little silly to implement something one way just to turn around and immediately change it

kymckay
kymckay approved these changes Jan 10, 2018
View reviewed changes
Copy link
Member

@kymckay kymckay left a comment

Choose a reason for hiding this comment

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

Approving based on consensus approval

@ulteq
Copy link
Contributor Author

ulteq commented Jan 10, 2018

why do it in a separate pull request when we could just do it in this one?

The massive cleanup would hide the actual code change.

@ulteq ulteq merged commit a5523bc into master Jan 10, 2018
@ulteq ulteq deleted the atragmx-bullet-mass-decimals branch January 10, 2018 18:44
@kymckay
Copy link
Member

kymckay commented Jan 10, 2018

The massive cleanup would hide the actual code change.

Oh I see now, I just meant on the one line where you'd changed the code. On the other hand, if we were going to change them all you'd probably want to do the cleanup in a separate commit anyway - thus the code change would remain visible.

@jonpas jonpas modified the milestones: 3.13.0, 3.12.1 Jan 15, 2018
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
@ulteq
ATagMX - Allow one decimal place for the bullet mass (in grams) (#6041)
0d1ffed 
Closes: #6040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonpas jonpas jonpas approved these changes

@kymckay kymckay kymckay approved these changes

Assignees
No one assigned
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Milestone
3.12.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ulteq @kymckay @jonpas