Skip to content

Correct a minor regression in Literal Numbers: remove duplicate exercise 'g' from LiteralNumbers (Floats section) #261

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

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

vivshaw
Copy link
Contributor

@vivshaw vivshaw commented Feb 13, 2021

Hello, I've noticed that PR #169 seems to have introduced a minor error in Literal Numbers: previously, the values in the Float section looked like this:

val a = 3.0
val b = 3.00
val c = 2.73
val d = 3f
val e = 3.22d
val f = 93e-9
val g = 93E-9
val h = 0.0
val i = 9.23E-9D

This version used both 93e-9 and 93E-9, to demonstrate that both are acceptable ways of representing the exponent. Furthermore, i used 9.23E-9D, to show that uppercase double literals work. But after #169, it looks like this:

val a = 3.0
val b = 3.00
val c = 2.73
val d = 3f
val e = 3.22d
val f = 93e-9
val g = 93e-9
val h = 0.0
val i = 9.23e-9d

In this version, both f and g are the same: 93e-9, and i has been swapped to lowercase. Unless I'm mistaken, this was probably not what's intended here! However, I see what caused this- looks like @juanpedromoreno was tidying up the codebase with some scalafmt magic after upgrading the version, and scalafmt stopped liking the uppercase "E" and "D". So, I would propose to fix this by setting literals.scientific and literals.double to Unchanged, allowing you to revert the Literal Numbers exercise to the prior format.

Alternatively, if you prefer to keep your .scalafmt.conf as-is, you could just remove the duplicate entry g in the Literal Numbers exercise and call it a day. If you prefer this, let me know, and I will revise my PR accordingly.

@vivshaw vivshaw force-pushed the master branch 3 times, most recently from 1f7502c to 8ba4d05 Compare February 13, 2021 07:54
@vivshaw vivshaw changed the title Correct a minor regression in Literal Numbers: return to using both "e" and "E" in the Float section Correct a minor regression in Literal Numbers: return to using both upper and lowercase literals in the Float section Feb 13, 2021
Copy link
Contributor

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Great catch, thanks for your time @vivshaw !

Copy link
Contributor

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Ooops, I've just realized that we couldn't change the .scalafmt file from here because it's automatically maintained by a CI pipeline (Github action), so it's going to be restored. Probably, removing g might be better. Thanks!

@vivshaw vivshaw changed the title Correct a minor regression in Literal Numbers: return to using both upper and lowercase literals in the Float section Correct a minor regression in Literal Numbers: remove duplicate exercise 'g' from LiteralNumbers (Floats section) Feb 15, 2021
@vivshaw
Copy link
Contributor Author

vivshaw commented Feb 16, 2021

Ooops, I've just realized that we couldn't change the .scalafmt file from here because it's automatically maintained by a CI pipeline (Github action), so it's going to be restored. Probably, removing g might be better. Thanks!

Alright @juanpedromoreno, thanks for the heads up! I've rewritten this PR to remove g instead. 👍

Copy link
Contributor

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @vivshaw !

@juanpedromoreno juanpedromoreno merged commit eeb45e9 into scala-exercises:master Feb 16, 2021
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.

2 participants