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

Printable Recipe Datatables #751

Merged
merged 10 commits into from
May 1, 2024

Conversation

ryan-hudson
Copy link
Contributor

What's changed?

Adds rewrite.exportDatatables property to Maven Plugin to allow Datatable information to be exported as a CSV.
See the Rewrite-Core PR for more information.

Anything in particular you'd like reviewers to focus on?

Note that the datatable output path is currently set to target/rewrite/datatables
These changes are dependent on the changes in this rewrite-core pr


if (exportDatatables) {
String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd_HH-mm-ss-SSS"));
Path datatableDirectoryPath = Paths.get("target/rewrite/", "datatables", timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use Paths.get("target", "rewrite", "datatables", timestamp); here instead? Might be safer for Windows users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good catch. Corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

So just to chime in, the Windows FileSystem implementation accepts both forward and back slashes without any issues. A *nix based FileSystem will only accept forward slashes, so either always using forward slashes, letting the filesystem choose (as illustrated by @ammachado), or using File.separatorChar are all valid solutions that work across OS boundaries.

@timtebeek timtebeek added the enhancement New feature or request label Mar 15, 2024
@ryan-hudson
Copy link
Contributor Author

@timtebeek Is there anything else needed to get this merged?

@timtebeek
Copy link
Contributor

hi @ryan-hudson thanks for the work! I've been out for a couple weeks, with a bit more travel to go still. Perhaps @knutwannheden or @sambsnyd can have a look.

@ryan-hudson
Copy link
Contributor Author

@timtebeek Bumping this again for visibility. It would be great to have this reviewed and merged!

@timtebeek timtebeek self-requested a review April 29, 2024 13:43
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks for the continued work on this, and the reminder that this was still waiting for review. I'm back again, so any followup should be quicker! :)

Some minor points that I think should be easy enough to address and see this through.

…o into new AbstractRewriteBaseRunMojo abstract class.

The extension path for the Abstract Run and Dry Run Mojos is now:
AbstractRewriteRunMojo/AbstractRewriteDryRunMojo -> AbstractRewriteBaseRunMojo -> AbstractRewriteMojo
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks for the rework! I'll have to review the moved changes still, but I imagine those were mostly a lift over right?

Co-authored-by: Tim te Beek <timtebeek@gmail.com>
@ryan-hudson
Copy link
Contributor Author

Thanks for the rework! I'll have to review the moved changes still, but I imagine those were mostly a lift over right?

Yep, purely copy paste work. No intentional changes to any of the moved methods.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ryan-hudson for the work both here and in

Should be need to see what others will build with data tables.

/cc @mike-solomon data tables are now no longer unique to Moderne, so we might want to document how to use this in OpenRewrite as well, as we have done for Moderne.

@timtebeek timtebeek merged commit a1ecdb4 into openrewrite:main May 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants