openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.03k stars 300 forks source link

Several recipe names and descriptions use `Type#method()` syntax #3664

Open Bananeweizen opened 8 months ago

Bananeweizen commented 8 months ago

Most recipes use the Type.method() syntax in their name and description. However, a minority contains Javadoc-like syntax using hash with Type#method() instead. I'm not sure if this worked at any time for the documentation generation, but in the tool itself (e.g. via maven plugin) the syntax with dot seems more readable for end users.

Example: https://docs.openrewrite.org/recipes/java/security/usefilescreatetempdirectory

Any opinion on how to normalize these? The issue affects multiple repositories.

Are you interested in contributing a fix to OpenRewrite?

Yes, but someone with authority needs to decide on which way to go. :)

timtebeek commented 8 months ago

Thanks a lot for noticing this inconsistency, and bringing it to our attention. I agree with you that consistency helps readability, and that the . form is more recognizable for folks when writing code, whereas the # form reminds me of JavaDoc. Probably best indeed to standardize on using ., and any help there welcome.

In terms of tackling this we could use a mechanism similar to RecipeMarkdownGenerator.kt to find offending instances. I would not propose we patch things there, but instead patch them in the recipes themselves in each module.

We could also expand our recipe display name and description validation in tests to disallow # https://github.com/openrewrite/rewrite/blob/2173a7125fd4360b71a998c70b6eb9b6431eedba/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java#L613-L615

Any additional thoughts on this @mike-solomon ?

timtebeek commented 8 months ago

Here's a quick attempt at automating this change with the Moderne platform: https://app.moderne.io/results/nnxlhGDJn

type: specs.openrewrite.org/v1beta/recipe
name: org.timtebeek.Example
displayName: Replace `#` with `.` in return statements
description: In Java files
recipeList:
  - org.openrewrite.text.FindAndReplace:
      find: (return ".*`.*)#(.*`.*";)
      replace: $1.$2
      regex: "True"
      caseSensitive: "False"
      multiline: "False"
      dotAll: "False"
      filePattern: "**/*.java"

The results would need to be checked and reviewed, but it seems like to good start for such a change.

mike-solomon commented 8 months ago

Thanks a lot for noticing this inconsistency, and bringing it to our attention. I agree with you that consistency helps readability, and that the . form is more recognizable for folks when writing code, whereas the # form reminds me of JavaDoc. Probably best indeed to standardize on using ., and any help there welcome.

In terms of tackling this we could use a mechanism similar to RecipeMarkdownGenerator.kt to find offending instances. I would not propose we patch things there, but instead patch them in the recipes themselves in each module.

We could also expand our recipe display name and description validation in tests to disallow #

https://github.com/openrewrite/rewrite/blob/2173a7125fd4360b71a998c70b6eb9b6431eedba/rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java#L613-L615

Any additional thoughts on this @mike-solomon ?

Everything you said makes sense to me. I also agree that standardizing on . over # would be ideal for readability/use.

Bananeweizen commented 8 months ago

@timtebeek I had seen at least one hyperlink with anchor in a description when I scanned through the findings. That "url#anchor" part would probably be wrongly replaced by your recipe.

timtebeek commented 8 months ago

@timtebeek I had seen at least one hyperlink with anchor in a description when I scanned through the findings. That "url#anchor" part would probably be wrongly replaced by your recipe.

Only if that URL is somewhere in between backticks right? I'd expect that to be rare, but we can carefully go over the recipe results above in the PRs. I did notice that we only replace the last instance right now, so we'd either need to be mindful of that in review, or do two / three passes of applying the recipe.