-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: unify create default values & stop project name transform
#1213
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1213 +/- ##
=========================================
Coverage ? 71.98%
=========================================
Files ? 21
Lines ? 1692
Branches ? 0
=========================================
Hits ? 1218
Misses ? 474
Partials ? 0
Continue to review full report at Codecov.
|
7ad4fe7 to
f955903
Compare
breautek
left a comment
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.
because there is no known limitation with it
I believe this was an artefact of when the project name was used for the package name as well, but that was changed in a recent version (I think v9), so I think changing this okay 👍
f955903 to
8b595d2
Compare
|
I rebased this PR Can you please go back to it, and same for the 2 linked PRs (apache/cordova-cli#554, apache/cordova-ios#1100) ? |
8b595d2 to
a17c050
Compare
erisu
left a comment
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 would suggest you revert the README.md changes and submit them in a seperate PR. These changes has nothing to do with the task of this PR.
Typically, the work from the PR should be focused on solving a single issue or task. If there was any issues with your PR and if we had to revert, all changes are reverted includiung the README.md.
I will also give you some feedback on the README so if you do make a new PR about it..
neither defined in doc nor in other platforms
…ache#1210 no known limitation with non-word characters, accents, unicode or spaces
a17c050 to
6addca1
Compare
create default values and stop project name transformcreate default values & stop project name transform
erisu
left a comment
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
create default values & stop project name transformcreate default values & stop project name transform
Platforms affected
Android
Motivation and Context
Mixed default values for
createbetween cli doc, cordova-android and cordova-ios.And wrong project name if with spaces and accents:
fixes #1210
Description
Package ids were mostly correct but not all, and same for project name. Suggested proper defaults:
io.cordova.helloCordovaHello CordovaSee linked PRs: apache/cordova-cli#554, apache/cordova-ios#1100
And
(see each commit for detail on my steps)
Testing
npm testsuccess + project creation, open and run from IDE into emulatorFine on 10.0.0-dev and same for 9.2.0-dev 👍 (would be nice to backport this into a 9.x fix release)
Checklist
(platform)if this change only applies to one platform (e.g.(android))