-
Notifications
You must be signed in to change notification settings - Fork 245
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): introduce submodules feature #1297
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Is this already magically supported by all code generators?
packages/jsii/lib/assembler.ts
Outdated
this._namespaceMap.set(symbol, ns); | ||
|
||
const decl = symbol.declarations?.[0]; | ||
if (decl != null |
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.
add a comment such as "if this is a supported jsii type"...
Also, do we want a warning if it's not?
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.
I don't think the warning is useful (exports that cannot be modeled by jsii are logged somewhere else as part of the next pass).
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
18d854b
to
bfcdc22
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 really review this. I wish you hadn't moved around all of the compliance test types which invalidated 100% of our snapshots.
If it's reasonable to ask, I would revert all changes to jsii-calc and friends and only add a few additional tests to verify submodules. This will make it much easier to actually review how this code behaves.
|
||
export * as ns1 from './namespaced'; | ||
|
||
export class Ns1 { |
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.
Would my_module
and myModule
collide as well?
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.
Yes. I guess I could add more coverage on this... Initially I only guarded against PascalCase
/ camelCase
collisions, but figured Java & Python would use snake_case
.
if (colliding != null) { | ||
const submoduleDecl = submodule.valueDeclaration ?? submodule.declarations[0]; | ||
this._diagnostic(node, ts.DiagnosticCategory.Error, | ||
`Submodule "${submodule.name}" conflicts with "${jsiiType.name}". Restricted names are: ${candidates.join(', ')}`, |
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.
I guess top level types should always be pascal case (interfaces, classes, enums), so I think by definition they can't conflict with namespace names...
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.
They can because the conflict space is PascalCase
(preferred for C#), camelCase
(preferred for JS), snake_case
(preferred for Python & Java).
packages/jsii-reflect/test/__snapshots__/jsii-tree.test.js.snap
Outdated
Show resolved
Hide resolved
Co-Authored-By: Elad Ben-Israel <[email protected]>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
export enum Goodness { | ||
/** It's pretty good */ | ||
PRETTY_GOOD, | ||
/** It's really good */ | ||
REALLY_GOOD, | ||
/** It's amazingly good */ | ||
AMAZINGLY_GOOD | ||
} |
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.
Optimistic today, aren't we? :-D
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.
} | ||
|
||
public addMember(member: PythonBase) { | ||
this.members.push(member); | ||
} | ||
|
||
public dependsOnModules(resolver: TypeResolver) { | ||
resolver = this.fqn ? resolver.bind(this.fqn, this.pythonName) : resolver; |
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.
this.boundResolver?
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.
That's not a thing on this particular type, due to peculiarities in the inheritance chain here.
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
1 similar comment
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Commit Message
feat(jsii): introduce submodules feature (#1297)
Introduces a
jsii
Submodule feature. Submodules are introducedby declaring a
namespace
:Submodules can be nested in other submodules, but the
dependency graph between submodules (and the top-level module)
must not result in a cycle (jsii` does currently not check for this
particular issue, although it probably will in the future).
Although this is out-of-scope for this change, they could be
enhanced so a specific
README.md
is attached to them, and sothat dedicated code-generating configuration can be specified.
Fixes #1286
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.