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: allow per-submodule naming configuration #1690

Merged
merged 5 commits into from
May 27, 2020

Conversation

RomainMuller
Copy link
Contributor

Introduces a new .jsiirc.json file leveraged by sumodules defined
using the export * as namespace from './namespace'; syntax. The
.jsiirc.json file is a sibling to what ./namespace resolves to (since
this typically resolves to ./namespace/index.ts, the .jsiirc.json file
must be at ./namespace/.jsiirc.json).

The targets key can je set within this .jsiirc.json document and
include specific configuration for various targets:

{
  "targets": {
    "dotnet": {
      // Only the "namespace" key is considered/supported at this point
      "namespace": "Some.DotNet.Namespace"
    },
    "java": {
      // Only the "package" key is considered/supported at this point
      "package": "some.java.package_name"
    },
    "python": {
      // Only the "module" key is considered/supported at this point
      // The configured module name for a submodule _MUST_ be nested
      // under the root module name for the package.
      "module": "some.module_name"
    }
  }
}

Related to #1286


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

Supports setting the `targets.python.module` configuration in submodules
via the `.jsiirc.json` file, and leverage this information during code
generation.
Allow providing a `targets.java.package` on submodules via entries in a
`.jsiirc.json` file, allowing customization of java pcakge names.
Allows customizing the .Net namespace used by submodules by adding new
`targets.dotnet.namespace` entries in the `.jsiirc.json` file.
@RomainMuller RomainMuller added effort/medium Medium work item – a couple days of effort module/pacmak Issues affecting the `jsii-pacmak` module contribution/core This is a PR that came from AWS. module/compiler Issues affecting the JSII compiler labels May 19, 2020
@RomainMuller RomainMuller requested a review from a team May 19, 2020 12:00
@RomainMuller RomainMuller self-assigned this May 19, 2020
@RomainMuller RomainMuller marked this pull request as ready for review May 19, 2020 12:11
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

Should the root package name be required in the .jsiirc.json file? Can it still be assumed so instead the custom submodule config for @scope/jsii-calc-lib/lib would be

{
  "targets": {
    "dotnet": {
      "namespace": "CustomSubmoduleName"
    },
    "java": {
      "package": "calculator.custom_submodule_name"
    },
    "python": {
      "module": "custom_submodule_name"
    }
  }
}

What happens if a user uses a root package name in one file that doesn't match the rest of the package? In effect, it makes sense to allow users to nest their submodule's output arbitrarily DEEPER but does it make sense to allow them to be output shallower? And if not why not remove that part of the config?

}
return parts.join('.');
return assm.targets!.dotnet!.namespace;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the 'non-nullability' of these fields not be represented in the assembly spec because of the presence of the dotnet target isn't guaranteed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is the spec doesn't actually represent per-language configurations... so they're opaque stuff entirely.

@mergify
Copy link
Contributor

mergify bot commented May 27, 2020

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 27, 2020
@mergify
Copy link
Contributor

mergify bot commented May 27, 2020

Merging (with squash)...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit b2aa424 into master May 27, 2020
@mergify mergify bot deleted the rmuller/submodule-config branch May 27, 2020 17:41
@mergify
Copy link
Contributor

mergify bot commented May 27, 2020

Merging (with squash)...

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. effort/medium Medium work item – a couple days of effort module/compiler Issues affecting the JSII compiler module/pacmak Issues affecting the `jsii-pacmak` module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants