Skip to content

fix: Package name collisions#22

Merged
eladb merged 20 commits intocdklabs:masterfrom
campionfellin:issue273
Aug 3, 2020
Merged

fix: Package name collisions#22
eladb merged 20 commits intocdklabs:masterfrom
campionfellin:issue273

Conversation

@campionfellin
Copy link
Copy Markdown
Contributor

Fixes: cdk8s-team/cdk8s#273

Signed-off-by: campionfellin campionfellin@gmail.com

Issue #, if available:

cdk8s-team/cdk8s#273

Description of changes:

Let consumers pass in a packageName to be used by JSII. Otherwise fall back to generated.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Fixes: cdk8s-team/cdk8s#273

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Comment thread src/compile.ts Outdated
Comment thread src/options.ts Outdated
@eladb
Copy link
Copy Markdown
Contributor

eladb commented Jul 28, 2020

@campionfellin Can you see if you can fix this but while you are at it?

cdk8s-team/cdk8s#274

…on package names

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Elad Ben-Israel and others added 3 commits July 29, 2020 10:58
Comment thread test/__snapshots__/srcmak.test.ts.snap Outdated

publication.publish()
",
"my_python_module/submodule/_jsii/pythonpackage@0.0.0.jsii.tgz": "��ko�H�����K���6��D�������TE�z����5�T����<]Ҫ���ݙ��Ύ��ih��p.;f��y�$�T(��
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be filtered out

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah... not really sure why my macbook keeps trying to add them... will try to fix

Copy link
Copy Markdown
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

See comment, also revert any projen/deps changes

Comment thread src/options.ts Outdated
* Package name
* @default - hash of the basepath to the module
*/
packageName?: string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure that's the correct name for this because users don't really interact with this value after code generation. Maybe moduleKey or something like that.

Docstring help needs to indicate that it should be project-unique.

Copy link
Copy Markdown
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

See some comments

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Comment thread src/compile.ts Outdated
targets.python = {
distName: 'generated',
module: options.python.moduleName,
module: options.python.moduleName.replace(/-/g, '_'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am assuming java would have a similar issue. I wonder if the correct behavior here is to validate per the language constraints and throw an error so the user will be required to pass in a valid name for the module.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah! We can do some validation instead of just correcting it. I'd be worried in some cases though where the user isn't the one explicitly choosing what this value is. In some use-cases, like CRDs in cdk8s from the internet, they might not control the group.

Would we then add this correction behavior to all consumers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can do a warn that lets the user know we're correcting it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the consumers of this library (eg cdk8s) should do the name mangling if needed. The constraints should be well documented

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok! I will add validation here then

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
@eladb eladb merged commit 766de96 into cdklabs:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple Imports Fail To Synth

2 participants