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

Document new jOOQ-refaster module #9825

Closed
lukaseder opened this issue Feb 12, 2020 · 22 comments
Closed

Document new jOOQ-refaster module #9825

lukaseder opened this issue Feb 12, 2020 · 22 comments

Comments

@lukaseder
Copy link
Member

The work from #9630 needs documentation in the manual

@knutwannheden
Copy link
Contributor

I think I have now finally figured out how to use Errorprone Refaster both for Java 8 and 11. I doubt that there is any solution for Java 6.

@lukaseder
Copy link
Member Author

I doubt that there is any solution for Java 6.

We don't need a Java 6 solution, that's fine. The jOOQ-checker module also doesn't support Java 6

@lukaseder
Copy link
Member Author

... A lot of this stuff depends on JSR-308, which shipped with Java 8

@knutwannheden
Copy link
Contributor

We need to make the compiled .refaster files available. We can of course add them to the ZIP distribution, but we should also make them available for download from the website and link them from the manual. Any suggestions for where in the ZIP and where on the web site to add the files?

@lukaseder
Copy link
Member Author

Can we load them from the classpath, too? Ideally, the files would be in a .jar file, so we could upload the .jar file to Maven Central like any other...

@knutwannheden
Copy link
Contributor

We could also commit the .refaster files to Git and link to the files in GitHub. But that would only work for .refaster files which we also include as part of the OSS edition.

Can we load them from the classpath, too? Ideally, the files would be in a .jar file, so we could upload the .jar file to Maven Central like any other...

So far I have not found any way to do that. Maybe I find something if I study the source code. I tested with URLs, but that doesn't work: google/error-prone#1513.

@lukaseder
Copy link
Member Author

We could also commit the .refaster files to Git and link to the files in GitHub. But that would only work for .refaster files which we also include as part of the OSS edition.

Hmm, not sure. We'd depend on GitHub's directory structures and the way they present URLs of files. I believe they have changed that a few times in the past.

I'd prefer a directory like www.jooq.org/refaster, similar to the XSD files. Also, we'd include them in the ZIP files somewhere, but I don't have an idea yet.

@knutwannheden
Copy link
Contributor

I'd prefer a directory like www.jooq.org/refaster, similar to the XSD files. Also, we'd include them in the ZIP files somewhere, but I don't have an idea yet.

Yes, that would also be my preferred solution. As with the XSDs we will want to have versioned files. So something like https://jooq.org/refaster/deprecation-3.13.0.refaster.

@lukaseder
Copy link
Member Author

Yes, that would also be my preferred solution. As with the XSDs we will want to have versioned files. So something like https://jooq.org/refaster/deprecation-3.13.0.refaster.

Exactly

@knutwannheden
Copy link
Contributor

Since Errorprone currently cannot be configured to load and apply multiple .refaster templates at once, I think it would for the time being make more sense to only produce a single .refaster file. At the moment I was planning to have separate deprecation.refaster and performance.refaster files. I think we should merge them into something like jooq.refaster or similar.

@lukaseder
Copy link
Member Author

I don't agree. We need separation of concerns. For example, everyone might agree with the deprecation transformations, but not with the performance ones. It is better for users to be able to apply these files individually and see the resulting patch output separately, rather than to have everything combined and then not be sure why some change was suggested.

@knutwannheden
Copy link
Contributor

OK. It is just really a shame that multiple files can't be applied at once. IIRC @Stephan202 said that he had a patch to address this. Possibly he could be coerced into providing a build for this 😉

@lukaseder
Copy link
Member Author

lukaseder commented Feb 17, 2020 via email

@lukaseder
Copy link
Member Author

lukaseder commented Feb 17, 2020 via email

@knutwannheden
Copy link
Contributor

Well, I see your point... how about publishing both? 1) a set of individual files and 2) the combined file?

That would also be an option. Due to another limitation of Refaster, a .refaster file can currently only be based on a single .java source file. So to create an all.refaster I would have to create a new .java source containing both of the other two .java sources. Also not very nice...

@lukaseder
Copy link
Member Author

lukaseder commented Feb 17, 2020 via email

@Stephan202
Copy link
Contributor

Very nice to see this take shape!

I think we have enough preprocessing infrastructure to produce such Java files by now 😉

That's probably the most pragmatic approach for now. Some alternatives for completeness:

I'd prefer a directory like www.jooq.org/refaster

Food for thought:

  • Since the .refaster files are (presumably) produced as part of a Maven build, you could also attach them as regular artifacts, e.g. using the build-helper-maven-plugin. This way they'll be hosted on Maven Central and keeping them in sync with the release cycles will come for "free".
  • It would be nice to (eventually) produce a Maven Central-hosted JAR in which these files are on the "classpath". More on that below :)

OK. It is just really a shame that multiple files can't be applied at once. IIRC @Stephan202 said that he had a patch to address this.

I checked and it seems I don't have a patch against Error Prone for this. (The fork includes google/error-prone#947, but while that is a step in the right direction, it is not enough.) Currently within Picnic we rely on the grouping provided by google/error-prone#552. The next step is to finalize the code I have described under point 2 in google/error-prone#552 (comment). This is reason I'm interested in the classpath-based distribution mechanism mentioned above: once the RefasterCheck is open-source users can not only apply Refaster rules, but they can also treat them as regular Error prone checks.

I'm swamped with work these weeks, but I can see how it'd be nice to finally open-source a first version of the code. Let me see what I can do in the upcoming weeks.

@knutwannheden
Copy link
Contributor

@Stephan202 Thanks for your feedback. There are indeed a few open questions and issues surrounding Refaster. I hope Google will at some point provide some feedback.

  • An alternative is to load all files using ObjectInputStreams, wrap them in a CompositeCodeTransformer and write them back out to using an ObjectOutputStream.

I just did this a few minutes ago 🤣

  • Since the .refaster files are (presumably) produced as part of a Maven build, you could also attach them as regular artifacts, e.g. using the build-helper-maven-plugin. This way they'll be hosted on Maven Central and keeping them in sync with the release cycles will come for "free".

Good idea. I will look into this. But I would really also prefer a solution which allows loading the .refaster files from the classpath.

knutwannheden added a commit that referenced this issue Feb 19, 2020
The `Splitter` now also copies the `jooq-refaster` and
`jooq-test-refaster` Maven modules for all relevant builds.
knutwannheden added a commit that referenced this issue Feb 19, 2020
`jOOQ-refaster` doesn't need to be built as part of the Java 8 builds
and in fact only causes problems.
knutwannheden added a commit that referenced this issue Feb 19, 2020
The `exec-maven-plugin` is supposed to use `${basedir}` as the working
directory (as one would expect), but it uses the parent process' working
directory. Also the existing `<workingDirectory>` parameter has no
effect...
@knutwannheden
Copy link
Contributor

Documentation has been published along with .refaster files. See https://www.jooq.org/doc/3.13/manual/tools/refaster/ and https://www.jooq.org/refaster/.

@lukaseder
Copy link
Member Author

  • Since the .refaster files are (presumably) produced as part of a Maven build, you could also attach them as regular artifacts, e.g. using the build-helper-maven-plugin. This way they'll be hosted on Maven Central and keeping them in sync with the release cycles will come for "free".

Good idea. I will look into this. But I would really also prefer a solution which allows loading the .refaster files from the classpath.

I'm not really a fan of this idea for these reasons:

  • It is very un-idiomatic to search for non jar/war/etc artifacts on Maven Central.
  • It feels hacky. While it may help with the distribution model, we cannot easily include the dependency for usage. Users would have to define a weird <type>refaster</type> or whatever dependency, and then reference it on their local file system nonetheless (as they wouldn't end up on the classpath)
  • It is very difficult to document to users who are not Maven gurus.
  • It is very difficult to keep forwards compatible. We might just rename the files, and then people would spend endless times looking for the current file name on Maven Central, as we'd be polluting our namespace.

As always, when ideas are "clever", the caveats start feeling like show stoppers. We just may have to accept that this technology is not very mature yet, and without the vendor (Google) providing clear guidelines and better features, I prefer not to do too many clever things.

@knutwannheden
Copy link
Contributor

Let's stick with the .refaster files on the web site and in the ZIP files for now. Once Error Prone can load the .refaster files from the classpath, we can revisit this.

@lukaseder
Copy link
Member Author

lukaseder commented Feb 21, 2020 via email

knutwannheden added a commit that referenced this issue Feb 21, 2020
This commit also contains whitespace changes due to changes required in
jOOQ's source preprocessor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants