multiple problems: Remove non-ASCII characters to fix #441

[robkeim]

#441

Originally tracked as #527

FYI @Insti @petertseng

[petertseng]

I mean, I'm not against removing this case, but if removing it in this commit then the commit message is no longer accurate - every character in this string is ASCII.

[robkeim]

Good catch @petertseng, I saw the unicode escape character and didn't double check if these were in the ASCII range.

I see a couple of choices here:

• Rename the PR/commit message (doesn't align with the original intent of the issue I was trying to fix)
• Revert this part of the change
• Revert this part of the change and replace the unicode representation of the characters with their hex equivalent

I'm in favor of option 3, what do you think?

Also I had a look at the description.md and I'll need to update it too:

You should use the following rules for the syntax: a number is a sequence of one or more (unicode) digits

I can update the PR tonight. What's the policy on making updates to commits? Do I keep the existing history and add a "integrating feedback" commit, or do I re-write the history so that the history looks "clean".

[petertseng]

replace the unicode representation of the characters with their hex equivalent

Is that possible? I don't think \x is allowed according to http://www.json.org/, though if you have information to the contrary, it would be good to have it. I suggest to take option 3 if possible, else take option 2.

Also I had a look at the description.md and I'll need to update it too:

Ah, true. Amusing that unicode digits is mentioned, since I didn't see any non-ascii digits being used in any language's test suite...

add a "integrating feedback" commit

I highly dislike those since the break the rule of "one logical change per commit" and the commit message tells me nothing about what changed.

Since the changes being made are small, I do not object to history being rewritten.

If there are many changes being made and then a reviewer has requested a small piece of feedback, then it is helpful to address the feedback in a separate commit simply so the reviewer can understand what has changed since the last review. But I would almost always squash those before merging, since it is likely those address-feedback commits do not stand on their own but only make sense in the context of the commit they amend.

These are my preferences; I don't know if the project as a whole has chosen some standards yet.

[robkeim]

Thanks @petertseng for the detailed response!

You were right about \x not being valid json so I went with the second option and reverted that change.

[robkeim]

@petertseng the new version is updated and ready for you to take a look at.

[petertseng]

fantastic.

[robkeim]

Thanks @petertseng! I'm assuming to submit this I should rebase and merge this to keep a linear commit history, is that correct?

Edit: actually looking at the history it seems that creating merge commits are favored.

[petertseng]

Hmm, from my observations, the Rebase and Merge button has the disadvantage that it's not possible to tell from the commit message what PR the commit is associated with - one has to go to the github web interface to know. For that reason I would prefer the merge commit.

[robkeim]

Ok thanks @petertseng!