-
Notifications
You must be signed in to change notification settings - Fork 162
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
Handle Java keywords for applicatoinIds #676
Conversation
- Check for Java keywords when testing. - Add _ before a keyword when generating. See #672 for details
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 % nits
@@ -34,6 +34,17 @@ const extractZipPromise = promisify(extractZip); | |||
const DISALLOWED_ANDROID_PACKAGE_CHARS_REGEX = /[^a-zA-Z0-9_\.]/g; | |||
const VALID_PACKAGE_ID_SEGMENT_REGEX = /^[a-zA-Z][A-Za-z0-9_]*$/; | |||
|
|||
// List of keywords for Java 11, as listed at | |||
// https://docs.oracle.com/javase/specs/jls/se11/html/jls-3.html#jls-3.9. | |||
const JAVA_KEYWORDS = [ |
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.
Sorry, but do you mind sorting these?
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.
LOL. I actually thought they were sorted, because I copied from the docs... but the docs use column-first sorting and this is using row first... I'll go change that.
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.
done
packages/core/src/lib/util.ts
Outdated
|
||
// Package names cannot contain Java keywords. The recommendation is adding an '_' before the | ||
// keyword. See https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html. | ||
if (JAVA_KEYWORDS.indexOf(part) >= 0) { |
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.
Can't we just use JAVA_KEYWORDS.includes(part)
?
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.
Oh, silly me... I remembered there was a handy method in JS Array, but couldn't remember the name. I checked the docs, but couldn't find what I was looking for (I think I was looking for something like contains
), and missed includes
... I'll change that.
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.
done
packages/core/src/lib/util.ts
Outdated
@@ -195,6 +214,13 @@ export function validatePackageId(input: string): string | null { | |||
} | |||
|
|||
for (const part of parts) { | |||
// Package names cannot contain Java keywords. The recommendation is adding an '_' before the | |||
// keyword. See https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html. | |||
if (JAVA_KEYWORDS.indexOf(part) >= 0) { |
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.
Same as above.
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.
likewise
- Sorts list of Java keywords. - Replaces indexOf with includes.
See #672 for details