Skip to content
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

Merged
merged 5 commits into from Aug 13, 2021

Conversation

ath0mas
Copy link
Contributor

@ath0mas ath0mas commented Apr 18, 2021

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:

  • id / package name: io.cordova.helloCordova
  • name / project name: Hello Cordova

See linked PRs: apache/cordova-cli#554, apache/cordova-ios#1100

And

  • Fix Wrong name in configuration for platform add android #1210: project name not transformed anymore (was replacing non-word characters with underscores), because there is no known limitation with it, project creation and all still work fine, and cordova-ios does it the same way
  • small update on main Readme to update/add details for use and test

(see each commit for detail on my steps)

Testing

npm test success + project creation, open and run from IDE into emulator

Fine 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

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@9c165cb). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1213   +/-   ##
=========================================
  Coverage          ?   71.98%           
=========================================
  Files             ?       21           
  Lines             ?     1692           
  Branches          ?        0           
=========================================
  Hits              ?     1218           
  Misses            ?      474           
  Partials          ?        0           
Impacted Files Coverage Δ
bin/lib/create.js 94.19% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c165cb...7ad4fe7. Read the comment docs.

Copy link
Contributor

@breautek breautek left a 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 👍

@breautek breautek requested a review from erisu April 19, 2021 01:03
@ath0mas ath0mas force-pushed the feature/create-defaults-android branch from f955903 to 8b595d2 Compare July 31, 2021 22:11
@ath0mas
Copy link
Contributor Author

ath0mas commented Aug 1, 2021

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) ?

@ath0mas ath0mas force-pushed the feature/create-defaults-android branch from 8b595d2 to a17c050 Compare August 2, 2021 09:59
Copy link
Member

@erisu erisu left a 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..

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/create.js Show resolved Hide resolved
@ath0mas ath0mas force-pushed the feature/create-defaults-android branch from a17c050 to 6addca1 Compare August 2, 2021 15:29
@ath0mas ath0mas requested a review from erisu August 2, 2021 21:41
@erisu erisu changed the title Review create default values and stop project name transform fix: unify create default values & stop project name transform Aug 13, 2021
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erisu erisu changed the title fix: unify create default values & stop project name transform feat: unify create default values & stop project name transform Aug 13, 2021
@erisu erisu merged commit 13bd3f4 into apache:master Aug 13, 2021
@ath0mas ath0mas deleted the feature/create-defaults-android branch August 19, 2021 21:24
wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong name in configuration for platform add android
4 participants