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(jsii): enforce enum names to be UPPER_CASE #541

Merged
merged 18 commits into from
Jun 20, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ like any other class.

> NOTE: Due to performance of the hosted JavaScript engine and marshaling costs,
__jsii__ modules are likely to be used for development and build tools, as
oppose to performance-sensitive runtime behavior.
opposed to performance-sensitive runtime behavior.

From Java:

Expand Down
4 changes: 2 additions & 2 deletions packages/jsii-calc-lib/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ export interface StructWithOnlyOptionals {
* See awslabs/jsii#138
*/
export enum EnumFromScopedModule {
Value1,
Value2
VALUE1,
VALUE2
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/jsii-calc-lib/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,13 @@
"docs": {
"stability": "deprecated"
},
"name": "Value1"
"name": "VALUE1"
},
{
"docs": {
"stability": "deprecated"
},
"name": "Value2"
"name": "VALUE2"
}
],
"name": "EnumFromScopedModule"
Expand Down Expand Up @@ -540,5 +540,5 @@
}
},
"version": "0.11.3",
"fingerprint": "jAkwT5aChzSZi3hTeylIFDPL1J8qUy++0B75C5vIA6Y="
"fingerprint": "iggTfEXzBxodU2TEDzkmrmsuEakOvA/UXmOA/8nQboU="
}
10 changes: 5 additions & 5 deletions packages/jsii-calc/lib/calculator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export namespace composition {
/**
* The .toString() style.
*/
public stringStyle = CompositeOperation.CompositionStringStyle.Normal
public stringStyle = CompositeOperation.CompositionStringStyle.NORMAL

/**
* A set of prefixes to include in a decorated .toString().
Expand All @@ -157,9 +157,9 @@ export namespace composition {

toString() {
switch (this.stringStyle) {
case CompositeOperation.CompositionStringStyle.Normal:
case CompositeOperation.CompositionStringStyle.NORMAL:
return this.expression.toString();
case CompositeOperation.CompositionStringStyle.Decorated:
case CompositeOperation.CompositionStringStyle.DECORATED:
return this.decorationPrefixes.join('') + this.expression.toString() + this.decorationPostfixes.join('');
default:
throw new Error(`Unknown string style: ${this.stringStyle}`);
Expand All @@ -173,10 +173,10 @@ export namespace composition {
*/
export enum CompositionStringStyle {
/** Normal string expression */
Normal,
NORMAL,

/** Decorated string expression */
Decorated
DECORATED
}
}
}
Expand Down
26 changes: 13 additions & 13 deletions packages/jsii-calc/lib/compliance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import base = require('@scope/jsii-calc-base');
const readFile = promisify(fs.readFile);

export enum AllTypesEnum {
MyEnumValue,
YourEnumValue = 100,
ThisIsGreat
MY_ENUM_VALUE,
YOUR_ENUM_VALUE = 100,
THIS_IS_GREAT
}

export enum StringEnum {
Expand Down Expand Up @@ -169,7 +169,7 @@ export class AllTypes {
// enum

public optionalEnumValue?: StringEnum;
private enumValue: AllTypesEnum = AllTypesEnum.ThisIsGreat;
private enumValue: AllTypesEnum = AllTypesEnum.THIS_IS_GREAT;

get enumProperty() {
return this.enumValue;
Expand All @@ -178,9 +178,9 @@ export class AllTypes {
set enumProperty(value: AllTypesEnum) {
this.enumValue = value;
switch (value) {
case AllTypesEnum.MyEnumValue:
case AllTypesEnum.YourEnumValue:
case AllTypesEnum.ThisIsGreat:
case AllTypesEnum.MY_ENUM_VALUE:
case AllTypesEnum.YOUR_ENUM_VALUE:
case AllTypesEnum.THIS_IS_GREAT:
return;
default:
throw new Error('Invalid enum: ' + value);
Expand Down Expand Up @@ -1008,7 +1008,7 @@ export interface ImplictBaseOfBase extends base.BaseProps {
* See awslabs/jsii#138
*/
export class ReferenceEnumFromScopedPackage {
public foo?: EnumFromScopedModule = EnumFromScopedModule.Value2;
public foo?: EnumFromScopedModule = EnumFromScopedModule.VALUE2;

public loadFoo(): EnumFromScopedModule | undefined {
return this.foo;
Expand Down Expand Up @@ -1594,7 +1594,7 @@ export abstract class PartiallyInitializedThisConsumer {

export class ConstructorPassesThisOut {
constructor(consumer: PartiallyInitializedThisConsumer) {
const result = consumer.consumePartiallyInitializedThis(this, new Date(0), AllTypesEnum.ThisIsGreat);
const result = consumer.consumePartiallyInitializedThis(this, new Date(0), AllTypesEnum.THIS_IS_GREAT);
if (result !== 'OK') {
throw new Error(`Expected OK but received ${result}`);
}
Expand Down Expand Up @@ -1702,13 +1702,13 @@ export class SingletonString {
private constructor() { }

public isSingletonString(value: string): boolean {
return value === SingletonStringEnum.SingletonString;
return value === SingletonStringEnum.SINGLETON_STRING;
}
}
/** A singleton string */
export enum SingletonStringEnum {
/** 1337 */
SingletonString = '3L1T3!'
SINGLETON_STRING = '3L1T3!'
}
/**
* Verifies that singleton enums are handled correctly
Expand All @@ -1718,11 +1718,11 @@ export enum SingletonStringEnum {
export class SingletonInt {
private constructor() { }
public isSingletonInt(value: number): boolean {
return value === SingletonIntEnum.SingletonInt;
return value === SingletonIntEnum.SINGLETON_INT;
}
}
/** A singleton integer. */
export enum SingletonIntEnum {
/** Elite! */
SingletonInt = 1337
SINGLETON_INT = 1337
}
12 changes: 6 additions & 6 deletions packages/jsii-calc/lib/stability.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ export class ExperimentalClass {
/** @experimental */
export enum ExperimentalEnum {
/** @experimental */
OptionA,
OPTION_A,
/** @experimental */
OptionB
OPTION_B
}

/** @stable */
Expand Down Expand Up @@ -64,9 +64,9 @@ export class StableClass {
/** @stable */
export enum StableEnum {
/** @stable */
OptionA,
OPTION_A,
/** @stable */
OptionB
OPTION_B
}

/** @deprecated it just wraps a string */
Expand Down Expand Up @@ -98,7 +98,7 @@ export class DeprecatedClass {
/** @deprecated your deprecated selection of bad options */
export enum DeprecatedEnum {
/** @deprecated option A is not great */
OptionA,
OPTION_A,
/** @deprecated option B is kinda bad, too */
OptionB
OPTION_B
}
28 changes: 14 additions & 14 deletions packages/jsii-calc/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -892,19 +892,19 @@
"docs": {
"stability": "experimental"
},
"name": "MyEnumValue"
"name": "MY_ENUM_VALUE"
},
{
"docs": {
"stability": "experimental"
},
"name": "YourEnumValue"
"name": "YOUR_ENUM_VALUE"
},
{
"docs": {
"stability": "experimental"
},
"name": "ThisIsGreat"
"name": "THIS_IS_GREAT"
}
],
"name": "AllTypesEnum"
Expand Down Expand Up @@ -2262,14 +2262,14 @@
"deprecated": "option A is not great",
"stability": "deprecated"
},
"name": "OptionA"
"name": "OPTION_A"
},
{
"docs": {
"deprecated": "option B is kinda bad, too",
"stability": "deprecated"
},
"name": "OptionB"
"name": "OPTION_B"
}
],
"name": "DeprecatedEnum"
Expand Down Expand Up @@ -2972,13 +2972,13 @@
"docs": {
"stability": "experimental"
},
"name": "OptionA"
"name": "OPTION_A"
},
{
"docs": {
"stability": "experimental"
},
"name": "OptionB"
"name": "OPTION_B"
}
],
"name": "ExperimentalEnum"
Expand Down Expand Up @@ -7179,7 +7179,7 @@
"stability": "experimental",
"summary": "Elite!"
},
"name": "SingletonInt"
"name": "SINGLETON_INT"
}
],
"name": "SingletonIntEnum"
Expand Down Expand Up @@ -7242,7 +7242,7 @@
"stability": "experimental",
"summary": "1337."
},
"name": "SingletonString"
"name": "SINGLETON_STRING"
}
],
"name": "SingletonStringEnum"
Expand Down Expand Up @@ -7338,13 +7338,13 @@
"docs": {
"stability": "stable"
},
"name": "OptionA"
"name": "OPTION_A"
},
{
"docs": {
"stability": "stable"
},
"name": "OptionB"
"name": "OPTION_B"
}
],
"name": "StableEnum"
Expand Down Expand Up @@ -8759,20 +8759,20 @@
"stability": "experimental",
"summary": "Normal string expression."
},
"name": "Normal"
"name": "NORMAL"
},
{
"docs": {
"stability": "experimental",
"summary": "Decorated string expression."
},
"name": "Decorated"
"name": "DECORATED"
}
],
"name": "CompositionStringStyle",
"namespace": "composition.CompositeOperation"
}
},
"version": "0.11.3",
"fingerprint": "PRTiHZmA0R0OqG2aleNqw5twteIgxeOUgjhzIkQMaxg="
"fingerprint": "ap5kT9z2b1y6ofObpCic6pDWcRTWCr0ivtq83hsi6ds="
}
2 changes: 1 addition & 1 deletion packages/jsii-calc/test/test.calc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ calc.neg(); assert.equal(-3200000, calc.value);

calc.curr = new Multiply(new calcLib.Number(2), calc.curr);
assert.equal(-6400000, calc.value);
calc.stringStyle = composition.CompositeOperation.CompositionStringStyle.Decorated;
calc.stringStyle = composition.CompositeOperation.CompositionStringStyle.DECORATED;
assert.equal(calc.toString(),
'<<[[{{(2 * -(((((1 * ((0 + 10) * 2)) * ((0 + 10) * 2)) * ((0 + 10) * 2)) * ((0 + 10) * 2)) * ((0 + 10) * 2)))}}]]>>');

Expand Down
22 changes: 11 additions & 11 deletions packages/jsii-diff/test/test.enums.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ export = {
async 'okay to add a member to an enum'(test: Test) {
await expectNoError(test, `
export enum Foo {
Bar,
Baz,
BAR,
BAZ,
}
`, `
export enum Foo {
Bar,
Baz,
Quux
BAR,
BAZ,
QUUX
}
`);
test.done();
Expand All @@ -26,17 +26,17 @@ export = {

async 'not okay to remove a member from an enum'(test: Test) {
await expectError(test,
/member Quux has been removed/,
/member QUUX has been removed/,
`
export enum Foo {
Bar,
Baz,
Quux
BAR,
BAZ,
QUUX
}
`, `
export enum Foo {
Bar,
Baz
BAR,
BAZ
}
`);
test.done();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,21 +260,21 @@ public void GetAndSetEnumValues()

calc.Add(9);
calc.Pow(3);
Assert.Equal(CompositionStringStyle.Normal, calc.StringStyle);
calc.StringStyle = CompositionStringStyle.Decorated;
Assert.Equal(CompositionStringStyle.Decorated, calc.StringStyle);
Assert.Equal(CompositionStringStyle.NORMAL, calc.StringStyle);
calc.StringStyle = CompositionStringStyle.DECORATED;
Assert.Equal(CompositionStringStyle.DECORATED, calc.StringStyle);
Assert.Equal("<<[[{{(((1 * (0 + 9)) * (0 + 9)) * (0 + 9))}}]]>>", calc.ToString());
}

[Fact(DisplayName = Prefix + nameof(EnumFromScopedModule))]
public void UseEnumFromScopedModule()
{
ReferenceEnumFromScopedPackage obj = new ReferenceEnumFromScopedPackage();
Assert.Equal(EnumFromScopedModule.Value2, obj.Foo);
obj.Foo = EnumFromScopedModule.Value1;
Assert.Equal(EnumFromScopedModule.Value1, obj.LoadFoo());
obj.SaveFoo(EnumFromScopedModule.Value2);
Assert.Equal(EnumFromScopedModule.Value2, obj.Foo);
Assert.Equal(EnumFromScopedModule.VALUE2, obj.Foo);
Copy link
Contributor

Choose a reason for hiding this comment

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

wait... what's the convention for .NET? Do we want .NET to also use all-caps?
Same question for Python.... @garnaat is it idiomatic to use all caps for enum values?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • .NET seems to prefer PascalCase.
  • Python looks like ALL_CAPS are good.

So basically, the .NET code generator needs to covert the case for .NET.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good question. I thought we wanted to avoid dropping into the language specific mangling... let me check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Pascal casing for both names and values are the idiomatic thing for .NET

Copy link
Contributor

Choose a reason for hiding this comment

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

No. We wanted to normalize the typescript/jsii source to CAPITALS but then, jsii is all about language-specific idiomacy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll make the changes to the generation of .NET so we can change enum values into PascalCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we decided we would make the changes for .NET generation to use enum members in PascalCase outside of this PR.

Should I create an issue for it before resolving this conversation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, "right click, create issue"

obj.Foo = EnumFromScopedModule.VALUE1;
Assert.Equal(EnumFromScopedModule.VALUE1, obj.LoadFoo());
obj.SaveFoo(EnumFromScopedModule.VALUE2);
Assert.Equal(EnumFromScopedModule.VALUE2, obj.Foo);
}

[Fact(DisplayName = Prefix + nameof(UndefinedAndNull))]
Expand Down Expand Up @@ -929,7 +929,7 @@ public override String ConsumePartiallyInitializedThis(ConstructorPassesThisOut
{
Assert.NotNull(obj);
Assert.Equal(new DateTime(0), dt);
Assert.Equal(AllTypesEnum.ThisIsGreat, ev);
Assert.Equal(AllTypesEnum.THIS_IS_GREAT, ev);

return "OK";
}
Expand Down
Loading