-
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
feat: unify create
default values & stop project name transform
#1213
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
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
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
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
create
between 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.helloCordova
Hello Cordova
See linked PRs: apache/cordova-cli#554, apache/cordova-ios#1100
And
(see each commit for detail on my steps)
Testing
npm test
success + 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)
)