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

Overload unimplemented getCharContent method #96

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Overload unimplemented getCharContent method #96

merged 1 commit into from
Apr 13, 2020

Conversation

onizucraft
Copy link

@onizucraft onizucraft commented Apr 9, 2020

It seems that to be able to integrate with JavaPoet code generation tool this little tweak is needed.


@Override
public CharSequence getCharContent(final boolean ignoreEncodingErrors) {
return new String(this.os.toByteArray(), Charset.forName("UTF-8"));
Copy link
Author

Choose a reason for hiding this comment

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

Probably the encoding can be obtained somewhere else.

Copy link

Choose a reason for hiding this comment

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

Btw, Charset.forName("UTF-8") can be replaced with StandardCharsets.UTF_8.

Copy link
Member

Choose a reason for hiding this comment

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

@since 1.7

We still support Java 6

Copy link
Member

Choose a reason for hiding this comment

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

... not in that class, though. So yes, it could be replaced. Will do

@lukaseder
Copy link
Member

What problem is this solving? Can you please add a description and/or a test case that would fail if you hadn't added this method?

@onizucraft
Copy link
Author

It solves issue 81, the second problem that was reported: #81 (comment)

@onizucraft
Copy link
Author

I'll try to setup a unit test.

@onizucraft
Copy link
Author

onizucraft commented Apr 9, 2020

It may be a problem with the integration with https://github.com/square/javapoet when using it to generate code in the Processor. It seems that when writing the source code, it delegates the creation of the JavaFileObject to the filer (https://github.com/square/javapoet/blob/44622704248975ca398b90b4385005fdddeb35f9/src/main/java/com/squareup/javapoet/JavaFile.java#L164).

Im not really sure how the internals are working, but it seems that the generated file is being handled by

public JavaFileObject getJavaFileForOutput(

I'm not sure if you want to maintain the integration with javapoet (I think it would be cool, plus it only needs this harmless tweak). I'm willing to add a unit test, but I'll have to add the javapoet dependency.

@ghost
Copy link

ghost commented Apr 12, 2020

@GabrielPS
I don't think there is a problem with JavaPoet. Filer is a standard and preferable way for reading and writing files in the Annotation Processor.

@ghost
Copy link

ghost commented Apr 12, 2020

@GabrielPS
Btw, you made a change in the "jOOR-java-8" module, but what about "jOOR" (for Java 9+, if I understand correctly)?
@lukaseder
Do you use some kind of generation tool and only support Java 9 implementation with tags ([java-8], [java-9], ...) or how does it all work?

@onizucraft
Copy link
Author

I did not try other Java versions, sorry.

@lukaseder
Copy link
Member

@GabrielPS
I don't think there is a problem with JavaPoet. Filer is a standard and preferable way for reading and writing files in the Annotation Processor.

Is there a simple way to call this method in a unit test without any third party assumptions? I think the suggested code is fine as it is, but I prefer not to accept any PRs without understanding what specific problem is being fixed.

Do you use some kind of generation tool and only support Java 9 implementation with tags ([java-8], [java-9], ...) or how does it all work?

Yes, there's a closed source preprocessor for these things.

I did not try other Java versions, sorry.

Don't worry about it. I'll take it from here, once I can see what problem this is solving.

@lukaseder
Copy link
Member

Is this it? #98, https://github.com/jbreathe/joor-repro

@lukaseder
Copy link
Member

I'll merge it without a test for now. If you can think of a test, feel free...

@lukaseder lukaseder reopened this Apr 13, 2020
@lukaseder lukaseder merged commit b86ebb8 into jOOQ:master Apr 13, 2020
@onizucraft
Copy link
Author

I tried to setup a test, but had no time. Sorry :(

@onizucraft onizucraft deleted the issue-81 branch April 13, 2020 12:24
@lukaseder
Copy link
Member

No worries, thanks again.

@ghost
Copy link

ghost commented Apr 13, 2020

@lukaseder

Is this it? #98, https://github.com/jbreathe/joor-repro

Yes, thank you!
I will close #98 in a minute.

Is there a simple way to call this method in a unit test without any third party assumptions?

I think yes. Give me a few minutes, I will try to change the code in my repro.

upd: something like this:

String rawSource = javaFile.toString();
try {
    javaFile.writeTo(processingEnv.getFiler());
    JavaFileObject implSourceFile = processingEnv.getFiler().createSourceFile(packageName + "." + implName);
    try (Writer writer = implSourceFile.openWriter()) {
        writer.append(rawSource);
    }
} catch (IOException e) {
    ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants