-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Major] Removed unnecessary project name restriction #859
[Major] Removed unnecessary project name restriction #859
Conversation
This PR does alter a function that is exported. While I saw no evidence that this PR will be a breaking change, there is still a possibility. Therefore this PR should only be applied on the next major (9.x) |
Codecov Report
@@ Coverage Diff @@
## master #859 +/- ##
==========================================
- Coverage 66.25% 66.17% -0.08%
==========================================
Files 19 19
Lines 1843 1839 -4
==========================================
- Hits 1221 1217 -4
Misses 622 622
Continue to review full report at Codecov.
|
…rting with a number. Project names starting with a number is perfectly valid.
73a9c14
to
adcd59e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a suggestion regarding the function's JSDoc, but other than that, this LGTM 👍
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
Just a reminder that this PR should only be merged once master has been upgraded to 9.x |
Yes, as the [major] title suggests - side note: I only approve but leave the merging to the author of the PR (unless it's no Apache org member) |
@breautek I think this PR is ready for merging after applying a rebase to fix the conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
Platforms affected
Android
Motivation and Context
There was a condition in
validateProjectName
that prevented the project name from started with a number. It was found that this was added approximately 5 years ago, and it was assumed that once upon a time, this was because project names were used to generate a java class, based on the comments. However, that no longer appears to be the case.Being able to support project names that starts with a number is important and perfectly valid (e.g: 8 Ball Pool).
It fixes #584 and fixes #701 where lots of people requested this change.
Description
Simply removed the condition preventing project names from starting with a number.
validateProjectName
has two other checks which were left untouched. I've also updated the comments to remove any references suggesting that successfulvalidateProjectName
calls means its safe to use the string as a java class name.There was a unit test test for failure for project names starting with numbers which have been removed. Two items were added for the
valid
list for valid name checks which includes names starting with numbers.Testing
Manual testing using:
cordova platform add
cordova build android
cordova run android
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)