Closed Bug 1589072 Opened 4 years ago Closed 4 years ago

Improve numeric separators error messages

Categories

(Core :: JavaScript Engine, task, P2)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: fs, Assigned: rohitawate121, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: lang=C++)

Attachments

(4 files, 1 obsolete file)

Bug 1589072 - Improve numeric separators error messages
47 bytes, text/x-phabricator-request
Details | Review
Bug 1589072 - Improve numeric separators error messages
47 bytes, text/x-phabricator-request
Details | Review
Bug 1589072 - Improve numeric separators error messages
47 bytes, text/x-phabricator-request
Details | Review
Bug 1589072 - Improve numeric separators error messages
47 bytes, text/x-phabricator-request
Details | Review

We seem to throw this in most cases:

  • "SyntaxError: missing digit after '_' numeric separator"

Chrome throws nicer messages:

  • "Numeric separators are not allowed at the end of numeric literals" for 1_
  • "Only one underscore is allowed as numeric separator" for 1__1

Separator after null is fine, I guess. At least it's handled with an own message by us as well:

  • Firefox: "numeric separators '_' are not allowed in numbers that start with '0'"
  • Chrome: "Numeric separator can not be used after leading 0."

I think what Chrome throws for 1_ and 1__1 is better. Should we create nicer error messages for these cases as well?

Priority: -- → P2

The fix is simple: check for '_' here and then replace the error message here with two different error messages.

Plus a test. Want to give it a shot?

Mentor: jorendorff
Keywords: good-first-bug
Whiteboard: lang=C++

Hi! I'm new around here. I would like to give this a shot.

(In reply to Jason Orendorff [:jorendorff] from comment #1)

The fix is simple: check for '_' here and then replace the error message here with two different error messages.

Plus a test. Want to give it a shot?
Hi Jason, I wanna make sure I understand the issue correctly.
We need to replace the error message to two different cases of error messages, So we need to separately check for successive '' and another check for '' at the end of numeric literals?

Flags: needinfo?(jorendorff)

(In reply to Ahmad Ganzouri from comment #3)

(In reply to Jason Orendorff [:jorendorff] from comment #1)

The fix is simple: check for '_' here and then replace the error message here with two different error messages.

Plus a test. Want to give it a shot?
Hi Jason, I wanna make sure I understand the issue correctly.
We need to replace the error message to two different cases of error messages, So we need to separately check for successive '_' and another check for'_' at the end of numeric literals?

Note Updated for '_' typo.

Flags: needinfo?(jorendorff)

(In reply to Ahmad Ganzouri from comment #4)

(In reply to Ahmad Ganzouri from comment #3)

(In reply to Jason Orendorff [:jorendorff] from comment #1)

The fix is simple: check for '_' here and then replace the error message here with two different error messages.

Plus a test. Want to give it a shot?
Hi Jason, I wanna make sure I understand the issue correctly.
We need to replace the error message to two different cases of error messages, So we need to separately check for successive '_' and another check for'_' at the end of numeric literals?

Note Updated for '_' typo.

Hi Ahmad! I've been working on this issue since Saturday. I already have a patch ready and have been awaiting Jason's green signal.

Rohit, just post your patch here. After that only mentors can review it.

Kind Regards
Sonia

Attached patch improve_num_sep_errmsg.patch (obsolete) — — Splinter Review

Thanks Sonia! I was of the impression that the bug needs to be assigned before submitting any patches.
Just submitted the patch. I hope I'm doing this correctly because I'm a bit confused. Am I supposed to be using Phabricator for patches and code reviews?

Depends on D50902

Hi Rohit,

Wait for the reviews now :) And that depends on you whether you want to use arc or moz-phab for submitting patches.

Hi Rohit, I think something went wrong the the patches. Can you merge these changes in a single patch? Thanks!

Thanks Sonia! :)
Hi Jan, yes I messed that up. Really sorry! This is the first time I'm working with Mercurial and Phabricator. Could you please guide me as to how I could merge the two patches?

(In reply to Rohit Awate from comment #13)

Hi Jan, yes I messed that up. Really sorry! This is the first time I'm working with Mercurial and Phabricator. Could you please guide me as to how I could merge the two patches?

No worries. How did you submit the patches? The recommended way is to use moz-phab and then you can amend commits and run moz-phab again to update the Phabricator patch, see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

I used moz-phab, however, I did not amend the first commit while adding some changes. Instead, I made a separate commit which ended up as a separate diff on Phabricator. How should I merge the patches on Phabricator now?
Thanks! :)

(In reply to Rohit Awate from comment #15)

I used moz-phab, however, I did not amend the first commit while adding some changes. Instead, I made a separate commit which ended up as a separate diff on Phabricator. How should I merge the patches on Phabricator now?

You should merge the patches locally. One option is to hg update to the first commit, make your changes, then hg commit --amend, then moz-phab.

(I'd recommend using the Mercurial evolve extension, it adds hg amend and some other useful commands.)

(In reply to Jan de Mooij [:jandem] from comment #16)

(In reply to Rohit Awate from comment #15)

I used moz-phab, however, I did not amend the first commit while adding some changes. Instead, I made a separate commit which ended up as a separate diff on Phabricator. How should I merge the patches on Phabricator now?

You should merge the patches locally. One option is to hg update to the first commit, make your changes, then hg commit --amend, then moz-phab.

(I'd recommend using the Mercurial evolve extension, it adds hg amend and some other useful commands.)

Thanks! I tried hg update followed by hg commit --amend, but it says abort: cannot amend changeset with children.
I tried another way on a dummy repository: hg strip --keep -r . which removes the last commit and then hg commit --amend. This did work. Do you think this will work well with Phabricator and if it is a good way?

Also, would pushing this patch to Phabricator remove the duplicate patch I had created before on there? (#D50904, specifically)

I'm sorry for asking these really stupid questions but I don't want to mess up on Phabricator again. Really appreciate your patience and help!

(In reply to Rohit Awate from comment #17)

Thanks! I tried hg update followed by hg commit --amend, but it says abort: cannot amend changeset with children.
I tried another way on a dummy repository: hg strip --keep -r . which removes the last commit and then hg commit --amend. This did work. Do you think this will work well with Phabricator and if it is a good way?

I think that will be fine. moz-phab should update the first revision. If it does create a new revision we can just close the first two and no harm is done.

Also, would pushing this patch to Phabricator remove the duplicate patch I had created before on there? (#D50904, specifically)

No, but you can just 'abandon' the second patch on Phabricator (look for the Add Action... box).

I'm sorry for asking these really stupid questions but I don't want to mess up on Phabricator again. Really appreciate your patience and help!

No problem! Thanks for working on this. You can also ask questions in the #jsapi channel on Mozilla's IRC server.

Also, for future changes, consider using the Mercurial evolve extension. hg amend does let you amend changesets with children.

Hi Jan, thanks for all the help! Really appreciate it.
I learned a lot in the process and plan to contribute regularly to Firefox henceforth, it feels really good.
Also thanks to Jason for the detailed description of the fix! :)

(In reply to Rohit Awate from comment #21)

Hi Jan, thanks for all the help! Really appreciate it.
I learned a lot in the process and plan to contribute regularly to Firefox henceforth, it feels really good.
Also thanks to Jason for the detailed description of the fix! :)

Fantastic! I flagged Jason as reviewer for the patch.

You can 'abandon' (close) the two other Phabricator patches here.

Thanks for helping, Jan.

Thanks for the patch, Rohit. This patch only changes one of the two error messages. I think we should change both.

Now I wonder if we can do even better than V8. "Numeric separator" is pretty jargony; if we say "underscore" it will probably be clearer what the error is about.

for 123_,
current: "missing digit after '_' numeric separator"
V8: "Numeric separators are not allowed at the end of numeric literals"
is this better?: "number can't end with an underscore"

for 1__234,
current: "missing digit after '_' numeric separator"
patch: "multiple numeric separators '_' are not allowed"
V8: "Only one underscore is allowed as numeric separator"
is this better?: "number can't contain multiple adjacent underscores"

I guess that does make sense considering the underscore is the only legal numeric separator.

Now that I think about it, "multiple numeric separators '_' are not allowed" might end up conveying that only one single underscore is allowed in a number literal. That is a bit ridiculous but I feel some people might interpret it as so.

Should I make the changes you proposed?

Yes, please!

Attachment #9104712 - Attachment is obsolete: true

Done! Are the error enum names okay?
This time I used hg commit --amend and yet moz-phab created a new revision. I'm confused. What am I doing wrong?

Hi Jason, thanks for accepting my patch! Do I need to add a checkin-needed tag to the revision on Phabricator?

Flags: needinfo?(jorendorff)

(In reply to Rohit Awate from comment #28)

Done! Are the error enum names okay?
This time I used hg commit --amend and yet moz-phab created a new revision. I'm confused. What am I doing wrong?

The names are fine. I think moz-phab looks for a Differential revision: line in your commit message. If it's not there, you'll get a new revision.

You can use hg log -v to see the full commit message (without -v, it just shows the first line).

I'll land this.

Flags: needinfo?(jorendorff)

OK, I didn't land it. I noticed a typo and requested a change.

Flags: needinfo?(rohitawate121)

OK, I didn't land it. I noticed a typo and requested a change.

Shoot! Fixed it.

The names are fine. I think moz-phab looks for a Differential revision: line in your commit message. If it's not there, you'll get a new revision. You can use hg log -v to see the full commit message (without -v, it just shows the first line).

Thanks! This worked.

Flags: needinfo?(rohitawate121)
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08d0bf739bad
Improve numeric separators error messages r=jorendorff

https://hg.mozilla.org/mozilla-central/rev/08d0bf739bad

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → rohitawate121

Bravo for this change!

(In reply to Sylvestre Ledru [:Sylvestre] from comment #35)

Bravo for this change!

Thanks Sylvestre! I messed up quite a bit; mostly because the contribution process is quite involved. However, I learned a lot and loved the welcoming, helpful and patient community. I hope to contribute regularly to Firefox henceforth. Also, I plan on blogging about this soon, so that new contributors won't repeat the same mistakes I made! ;)

Once again, thanks to Jason, Jan and Sonia! :)

You need to log in before you can comment on or make changes to this bug.