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

[Major] Removed unnecessary project name restriction #859

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Nov 2, 2019

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 successful validateProjectName 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

  • 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

@breautek
Copy link
Contributor Author

breautek commented Nov 2, 2019

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)

@breautek breautek added this to the 9.0.0 milestone Nov 2, 2019
@codecov-io
Copy link

codecov-io commented Nov 2, 2019

Codecov Report

Merging #859 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
bin/lib/create.js 92.54% <ø> (-0.19%) ⬇️

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 0e6ad28...0e85c2d. Read the comment docs.

bin/lib/create.js Outdated Show resolved Hide resolved
…rting with a number. Project names starting with a number is perfectly valid.
@breautek breautek force-pushed the gh-584-remove-project-number-check branch from 73a9c14 to adcd59e Compare November 13, 2019 03:05
bin/lib/create.js Outdated Show resolved Hide resolved
Copy link
Contributor

@raphinesse raphinesse left a 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>
@breautek breautek changed the title Removed unnecessary project name restriction [Major] Removed unnecessary project name restriction Nov 14, 2019
@breautek
Copy link
Contributor Author

Just a reminder that this PR should only be merged once master has been upgraded to 9.x

@timbru31
Copy link
Member

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)

@erisu erisu added this to Reviewer approved in Release Plan - 9.1.0 Jan 6, 2020
@erisu
Copy link
Member

erisu commented Jan 7, 2020

@breautek I think this PR is ready for merging after applying a rebase to fix the conflict.

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

@breautek breautek merged commit 91d2716 into apache:master Jan 7, 2020
Release Plan - 9.1.0 automation moved this from Reviewer approved to Done Jan 7, 2020
@breautek breautek deleted the gh-584-remove-project-number-check branch January 7, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
5 participants