Correct messages regarding sketch/library folder name restrictions #7734
Conversation
| tr("The sketch name had to be modified. Sketch names can only consist\n" + | ||
| "of ASCII characters and numbers and be less than 64 characters long."); | ||
| tr("The sketch name had to be modified. Sketch names can only consist of\n" + | ||
| "ASCII characters and numbers, no spaces, and be less than 64 characters long."); |
There was a problem hiding this comment.
Is this really correct? ASCII characters would include all 128 defined ASCII characters, but the actual limits are smaller: https://github.com/per1234/Arduino/blob/9edfa14531a9d184e36cc5761d4b18984daa3d1b/arduino-core/src/processing/app/BaseNoGui.java#L853-L857
Perhaps this message should just spell out the requirements exactly: A letter or number, followed by letters, numbers, dashes, dots and underscores (underscores are not listed in the whitelist, but anything else is replaced by an underscore, making the underscore implicitly allowed). Maximum length is 63 characters.
There was a problem hiding this comment.
Great idea. Thanks! I have implemented your suggestion via a force push. I ended up just squashing everything into a single commit since it ends up being a complete rewrite of both messages, rather than changing some specific wording. I like the use of the same wording in both messages, which should make any future changes to them easier. I suppose it would be best practices to just share the same string between the two so they never again get out of sync? Anyway, that's beyond my skills.
There was a problem hiding this comment.
@matthijskooijman when you get some time, please check my changes to see if this PR now meets your approval. Thanks!
| + "Library names must contain only basic letters and numbers.\n" | ||
| + "(ASCII only and no spaces, and it cannot start with a number)"), | ||
| + "Library folder names must contain only basic letters and numbers\n" | ||
| + "(ASCII only and no spaces), and be less than 64 characters long."), |
There was a problem hiding this comment.
The same constraints hold here.
There was a problem hiding this comment.
Looks good to me now. Two more nitpicks:
- There is a missing opening parenthesis in
- This restriction was removed by 4545283). - I would suggest a slight rewording: "Sketch names must start with a letter or number, followed by...". IMO this makes it more clear that the letter or number is about the first character only.
- Specify that library name error is about folder name. - We would normally expect "library name" to mean the "fancy name" (as defined by the library.properties name field). - Specify exactly which characters are allowed. - State that spaces are prohibited in sketch folder name. - Remove outdated message about library folders not being allowed to start with a number. - This restriction was removed by 4545283. - State library folder name length restriction. - Make sketch and library messages consistent with each other.
|
"Nitpicks" resolved via a force push. I also like that wording better for the messages. I appreciate your attention to detail regarding clean commits and clean commit messages. One of your comments years ago brought these concepts to my awareness. As someone who regularly ends up digging around through commit histories trying to understand the purpose of some code I really get why it matters. |
:-D |
Fixes #7613