Skip to content

feat: Java support#2

Merged
eladb merged 5 commits intocdklabs:masterfrom
campionfellin:java-support
Jun 2, 2020
Merged

feat: Java support#2
eladb merged 5 commits intocdklabs:masterfrom
campionfellin:java-support

Conversation

@campionfellin
Copy link
Copy Markdown
Contributor

Issue #, if available:

#1

Description of changes:

  • Updates README with contributing tips
  • Adds java support to this package

TODO: the java that's output into the snapshots looks good to me, but I want to yarn link and test with cdk8s first.

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

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
@campionfellin
Copy link
Copy Markdown
Contributor Author

@eladb snapshot tests will continue to fail due to generated date in the

@javax.annotation.Generated(value = \\"jsii-pacmak/1.5.0 (build 46538f8)\\", date = \\"2020-05-27T04:36:59.445Z\\") annotation.

Is it worth it to add a flag to jsii-pacmak like --no-annotations or --no-date-annotation?

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.

Let's add a feature to snapshotDirectory that can filter out certain lines from the snapshotted files to address the timestamp issue:

await snapshotDirectory(target, {
  excludeLines: [ /@javax.annotation.Generated/ ],
  excludeFiles: [ /generated@0.0.0.jsii.tgz/ ]
});

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md
Comment thread bin/jsii-srcmak.ts Outdated
Comment thread bin/jsii-srcmak.ts Outdated
Comment thread lib/options.ts Outdated
Comment thread test/cli.test.ts Outdated
Comment thread lib/compile.ts Outdated
@eladb eladb changed the title Java support feat: Java support May 27, 2020
…ptions

Signed-off-by: campionfellin <campionfellin@gmail.com>
Signed-off-by: campionfellin <campionfellin@gmail.com>
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.

Minor comments

Comment thread README.md
The output directory will include a java module that corresponds to the
original module. This code depends on the following maven package (should be defined directly or indirectly in the project's `pom.xml` file):

- [jsii](https://mvnrepository.com/artifact/software.amazon.jsii)
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.

Also worth mentioning that the output will include a tarball resource that must be bundled in the project (maybe provide an example snippet from a pom.xml?)

Comment thread lib/options.ts

/**
* Produce java code.
* @default - java is not generated
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.

Add a note here that says that the code will be produced under src (with an example of the full path combined with the package name). Also mention the requirements from Pom.xml (dependency on jsii runtime and resource).

Comment thread lib/srcmak.ts Outdated

if (options.java) {
const reldir = options.java.package.replace(/\./g, '/');
const source = path.resolve(path.join(workdir, 'dist/java/src/main/java', reldir));
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.

Why is it src/main? Is this where things are expected conventionally by the Pom.xml?

Comment thread test/util.ts Outdated
import * as fs from 'fs-extra';
import * as path from 'path';

interface excludeOptions {
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.

Suggested change
interface excludeOptions {
interface SnapshotOptions {

Comment thread test/util.ts Outdated
* file contents. Useful to perform snapshot testing against full directories.
*/
export async function snapshotDirectory(basedir: string, exclude: string[] = [], reldir = '.'): Promise<Record<string, string>> {
export async function snapshotDirectory(basedir: string, excludeOptions: excludeOptions = {}, reldir = '.'): Promise<Record<string, 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.

Suggested change
export async function snapshotDirectory(basedir: string, excludeOptions: excludeOptions = {}, reldir = '.'): Promise<Record<string, string>> {
export async function snapshotDirectory(basedir: string, options: SnapshotOptions = {}, reldir = '.'): Promise<Record<string, string>> {

Signed-off-by: campionfellin <campionfellin@gmail.com>
@eladb eladb merged commit 9e84d6e into cdklabs:master Jun 2, 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.

2 participants