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(submodules): allow python submodule name configuration #1502

Closed
wants to merge 1 commit into from

Conversation

RomainMuller
Copy link
Contributor

Using a .jsiirc.json file, the name generation for entities within a
submodule can be configured ot use a different name than the default.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Using a `.jsiirc.json` file, the name generation for entities within a
submodule can be configured ot use a different name than the default.
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 8, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 43a514b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

)
from .deeply_nested import (
INamespaced as _INamespaced_e2f386ad,
)
import scope.jsii_calc_base
import scope.jsii_calc_base_of_base
import scope.jsii_calc_lib

__jsii_assembly__ = jsii.JSIIAssembly.load("jsii-calc", "0.0.0", "jsii_calc", "[email protected]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to also fix #1501 as part of this commit (i.e. always import the root module so you can use __name__ instead of an explicit name in the .load() call?

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've actually fixed it separately because this would land too late.

venv="${outdir}/.env"
python3 -m venv ${venv}
. ${venv}/bin/activate
pip install pip~=20.0.2 setuptools~=46.1.3 wheel~=0.34.2 twine~=3.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do these version numbers come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

THey're the "latest" from pypi. Just some housekeeping since those aren't managed by dependabot.

{
"targets": {
"java": {
"package": "software.amazon.jsii.tests.calculator.refers_to_parent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to also support java and .net in this PR or as a separate PR?

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 was thinking probably subsequent PR, not settled yet.

*
* @default none
*/
submodules?: { [name: string]: AssemblyConfiguration };
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires updating the spec document, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hence this is a draft PR at this point :)

@@ -1168,3 +1168,8 @@ def test_collection_of_interfaces_map_of_structs():
def test_collection_of_interfaces_map_of_interfaces():
for elt in InterfaceCollections.map_of_interfaces().values():
assert getattr(elt, "ring") is not None

def test_load_submodules():
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to verify that they can be loaded out of order?

private readonly _submoduleMap = new Map<ts.Symbol, ts.Symbol>();
private readonly _submodules = new Set<ts.Symbol>();
// eslint-disable-next-line @typescript-eslint/explicit-member-accessibility
readonly #submoduleMap = new Map<ts.Symbol, ts.Symbol>();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use private?

Copy link
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.

Another important requirement which I hope we can include in this PR: we need to be able to provide an alternative name in each language for types in the root namespace as well.

In monocdk we publish the core types (e.g. App, Stack) under the root namespace, but in e.g. java, those are available under software.amazon.awscdk.core.App.

Copy @MrArnoldPalmer

@RomainMuller
Copy link
Contributor Author

@eladb - can't you just make monocdk's root Java package be software.amazon.awscdk.core, and then customize the Java package name of submodules in there? What's missing?

@eladb
Copy link
Contributor

eladb commented Apr 14, 2020

@eladb - can't you just make monocdk's root Java package be software.amazon.awscdk.core, and then customize the Java package name of submodules in there? What's missing?

I guess you are right. @MrArnoldPalmer can you acknowledge that you saw this discussion?

@RomainMuller
Copy link
Contributor Author

I'm dropping this PR because it'll be superceded by a different (and I believe slightly simpler) way to model the data in the Assembly schema.

@RomainMuller RomainMuller deleted the rmuller/submodules-model branch April 16, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants