Replace week year (YYYY) with year (yyyy) in date formats #129

AlekSimpson · 2023-06-28T00:19:46Z

What's changed?

This PR introduces a new recipe to replace date formats that use week year (YYYY) with the year format (yyyy).

What's your motivation?

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

I got the first test to pass but the second one I found a little more tricky. I tried using a visitMethodInvocation visitor for it. However, the .format() invocation was being found instead of the ofPattern call. I tried getting the select of the method which did give me the format invocation but getting the visitor to account for the case when only ofPattern is called was tough. Do you think this is the right approach for this test? Maybe I should try using a visitor like visitLiteral and then checking if the literal is passed into an ofPattern invocation instead?

Anyone you would like to review specifically?

@timtebeek

Any additional context

RSPEC-3986

Checklist

I've added unit tests to cover both positive and negative cases
I've added the license header to any new files through ./gradlew licenseFormat
I've used the IntelliJ auto-formatter on affected files
I've updated the documentation (if applicable)

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

timtebeek · 2023-06-28T09:38:14Z

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

+                    System.out.println("getting here");
+
+                    return JavaTemplate.builder("new SimpleDateFormat(#{any(java.lang.String)})")
+                            .contextSensitive()


I'm not entirely sure about the contextSensitive here; what made you add that?

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYearTest.java

timtebeek

Some initial feedback already. Needs a bit more work I think, but good start.

src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYearTest.java

…thYear.java Co-authored-by: Tim te Beek <[email protected]>

…thYearTest.java Co-authored-by: Tim te Beek <[email protected]>

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

… flexible

AlekSimpson · 2023-06-28T18:18:56Z

I added a couple more tests to test cases like "YY" being used in format patterns. I think this visitLiteral visitor should be able to cover all cases where a date is being formatted.

timtebeek · 2023-06-29T18:17:44Z

Added 167ef5e to demonstrate an issue with the current implementation; you're replacing YY in any string, not just those used as date format. We'd need that restriction before we can consider a merge.

AlekSimpson · 2023-06-29T18:30:56Z

Added 167ef5e to demonstrate an issue with the current implementation; you're replacing YY in any string, not just those used as date format. We'd need that restriction before we can consider a merge.

I thought the preconditions would stop it from running on the wrong strings. I ran across code samples where the string pattern would often be in just a standalone string variables. Other using preconditions to make sure the Date libraries are being used, I could also maybe add some extra logic to check for a larger date pattern after finding YY?

timtebeek · 2023-06-29T18:40:49Z

I thought the preconditions would stop it from running on the wrong strings. I ran across code samples where the string pattern would often be in just a standalone string variables. Other using preconditions to make sure the Date libraries are being used, I could also maybe add some extra logic to check for a larger date pattern after finding YY?

The preconditions determine whether to run on a compilation unit (typically file?) I believe; not on individual Strings.

I think we'd want to use the cursor here to find if there's a wrapping method invocation around the String, and if that matches any of the methods we cover here. So start with getCursor().find... and have autocompletion guide you from there, also looking at other similar uses in other recipes. Does that help?

AlekSimpson · 2023-06-29T21:18:29Z

I couldn't find any method on getCursor() that started with .find..., nothing was coming up in the autocomplete. However, I did realize that same thing could be achieved with cursor messaging by leaving a message on the methods/constructors that take in the string literals we are looking for.

timtebeek · 2023-06-30T07:20:46Z

Noticed there's now two failing tests; are you planning to pick those up and should I wait with a review?

ReplaceWeekYearWithYearTest > datePatternIsVariable() FAILED
    java.lang.AssertionError: Expected recipe to complete in 1 cycle, but took 0 cycle.
        at org.openrewrite.staticanalysis.ReplaceWeekYearWithYearTest.datePatternIsVariable(ReplaceWeekYearWithYearTest.java:153)

ReplaceWeekYearWithYearTest > changeSimpleDateFormat() FAILED
    java.lang.AssertionError: Expected recipe to complete in 1 cycle, but took 0 cycle.
        at org.openrewrite.staticanalysis.ReplaceWeekYearWithYearTest.changeSimpleDateFormat(ReplaceWeekYearWithYearTest.java:39)

AlekSimpson · 2023-06-30T14:34:47Z

ngeSimpleDateFor

Yes I will be picking those up. I was kind of stuck on them yesterday but now I have a new idea that maybe will help. Although I'm still not sure how we will know to edit formats that are assigned to variables. I could visit Identifiers as well but is there a way to access the identifiers value?

timtebeek · 2023-06-30T14:51:16Z

We do have some data flow analysis support that I've not used before myself; you can try to look for examples or ask Jonathan Leitschuh, although it's also fine to for now leave out cases where the format strings are in variables or constants, and only focus on direct use of literals going into the methods identified here.

…dded more tests

timtebeek · 2023-07-02T10:36:53Z

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

+                Cursor c = getCursor().dropParentWhile(is -> is instanceof J.Parentheses || !(is instanceof Tree));
+                if (c.getMessage("KEY") != null) {
+                    String value = li.getValueSource();
+                    if (value != null && value.contains("YY")) {


Why the check specifically here for at least YY? I'm reading the specification and it seems to allow for single Y as well.. 🤔

For parsing with the abbreviated year pattern ("y" or "yy"), SimpleDateFormat must interpret the abbreviated year relative to some century. It does this by adjusting dates to be within 80 years before and 20 years after the time the SimpleDateFormat instance is created. For example, using a pattern of "MM/dd/yy" and a SimpleDateFormat instance created on Jan 1, 1997, the string "01/11/12" would be interpreted as Jan 11, 2012 while the string "05/04/64" would be interpreted as May 4, 1964. During parsing, only strings consisting of exactly two digits, as defined by Character.isDigit(char), will be parsed into the default century. Any other numeric string, such as a one digit string, a three or more digit string, or a two digit string that isn't all digits (for example, "-1"), is interpreted literally. So "01/02/3" or "01/02/003" are parsed, using the same pattern, as Jan 2, 3 AD. Likewise, "01/02/-3" is parsed as Jan 2, 4 BC.
Otherwise, calendar system specific forms are applied. For both formatting and parsing, if the number of pattern letters is 4 or more, a calendar specific long form is used. Otherwise, a calendar specific short or abbreviated form is used.

And we might need to guard against the (quite hypothetical) use single quotes

Text can be quoted using single quotes (') to avoid interpretation. "''" represents a single quote. All other characters are not interpreted; they're simply copied into the output string during formatting or matched against the input string during parsing.

Ok the latest commits should address these issues

knutwannheden · 2023-07-05T11:11:04Z

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

+
+public class ReplaceWeekYearWithYear extends Recipe {
+    public static final MethodMatcher SIMPLE_DATE_FORMAT_CONSTRUCTOR_MATCHER = new MethodMatcher("java.text.SimpleDateFormat <constructor>(..)");
+    public static final MethodMatcher OF_PATTERN_MATCHER = new MethodMatcher("java.time.format.DateTimeFormatter ofPattern(..)");


Detail: I suggest declaring these constants as private. API surface area is always easy to extend, but more difficult to shrink.

knutwannheden · 2023-07-05T11:20:12Z

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

+
+                    String newValue = replaceY(value);
+
+                    return li.withValueSource(newValue).withValue(newValue);


This looks a bit wrong to me. The valueSource will be the value with quotes (possibly even Java 17 block quotes, for which we should have a test probably), while the value is without quotes. We should update it correctly here.

knutwannheden · 2023-07-05T11:24:11Z

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java

+                }
+            }
+
+            return output.toString();


I suspect that when we do the J.Literal#withValue() call it may end up producing a new J.Literal object. So I think we should here in the loop update a boolean variable if there was any change made and only then return this result. Otherwise the input value to this method. Does that make sense to you?

Ah I see, ya I can add a check to see if there are any changes actually being made.

AlekSimpson · 2023-07-05T22:43:13Z

Ok I've added another test case and made those last changes.

…ithYear

knutwannheden · 2023-07-06T10:16:49Z

@AlekSimpson I did a git merge origin/main as the changes in this PR listed a lot of changes which had previously been merged to the main branch.

knutwannheden

LGTM 👍

This review was on the initial commit. I have addressed all the feedback from it and made changes accordingly.

AlekSimpson added 2 commits June 27, 2023 13:33

creating new recipe to replace Week Year with Year formats

2b6a115

first test passes

9cf2f77

AlekSimpson added the recipe label Jun 28, 2023

AlekSimpson requested a review from timtebeek June 28, 2023 00:19

AlekSimpson self-assigned this Jun 28, 2023

AlekSimpson marked this pull request as draft June 28, 2023 00:19

AlekSimpson linked an issue Jun 28, 2023 that may be closed by this pull request

RSPEC-3986: Week Year ("YYYY") should not be used for date formatting #58

Closed

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java Show resolved Hide resolved

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java Outdated Show resolved Hide resolved

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java Outdated Show resolved Hide resolved

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java Outdated Show resolved Hide resolved

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYearTest.java Outdated Show resolved Hide resolved

timtebeek previously requested changes Jun 28, 2023

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYearTest.java Outdated Show resolved Hide resolved

src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYearTest.java Outdated Show resolved Hide resolved

AlekSimpson and others added 4 commits June 28, 2023 07:24

Update src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWi…

355341a

…thYear.java Co-authored-by: Tim te Beek <[email protected]>

Update src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWi…

4cbfe7c

…thYearTest.java Co-authored-by: Tim te Beek <[email protected]>

Update src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWi…

974c211

…thYearTest.java Co-authored-by: Tim te Beek <[email protected]>

Update src/test/java/org/openrewrite/staticanalysis/ReplaceWeekYearWi…

c4bd801

…thYearTest.java Co-authored-by: Tim te Beek <[email protected]>

timtebeek changed the title ~~Alek/replace week year with year~~ Replace week year (YYYY) with year (yyyy) in date formats Jun 28, 2023

timtebeek reviewed Jun 28, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/ReplaceWeekYearWithYear.java Outdated Show resolved Hide resolved

all tests pass, updated tests, changed visitor type, made recipe more…

5244382

… flexible

added license headers

03f886b

timtebeek marked this pull request as ready for review June 29, 2023 17:04

updated recipe so that it does not run on all strings

42289b1

AlekSimpson force-pushed the alek/ReplaceWeekYearWithYear branch from 167ef5e to 42289b1 Compare June 29, 2023 21:00

updated recipe to be selective with which literals are changed, and a…

71e25a0

…dded more tests

timtebeek reviewed Jul 2, 2023

View reviewed changes

AlekSimpson added 2 commits July 3, 2023 07:37

Update ReplaceWeekYearWithYear.java

47b48b5

Update ReplaceWeekYearWithYearTest.java

9b1f649

knutwannheden reviewed Jul 5, 2023

View reviewed changes

AlekSimpson added 4 commits July 5, 2023 16:13

fixed a weird issue

384c8e2

update

7d977be

removed unneeded DS_Store file from repo

5211ebe

update

658a332

Merge remote-tracking branch 'origin/main' into alek/ReplaceWeekYearW…

4507667

…ithYear

knutwannheden approved these changes Jul 6, 2023

View reviewed changes

timtebeek merged commit d96a05a into main Jul 6, 2023

timtebeek deleted the alek/ReplaceWeekYearWithYear branch July 6, 2023 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace week year (YYYY) with year (yyyy) in date formats #129

Replace week year (YYYY) with year (yyyy) in date formats #129

AlekSimpson commented Jun 28, 2023 •

edited

Loading

timtebeek Jun 28, 2023

timtebeek left a comment

AlekSimpson commented Jun 28, 2023

timtebeek commented Jun 29, 2023

AlekSimpson commented Jun 29, 2023 •

edited

Loading

timtebeek commented Jun 29, 2023

AlekSimpson commented Jun 29, 2023

timtebeek commented Jun 30, 2023

AlekSimpson commented Jun 30, 2023 •

edited

Loading

timtebeek commented Jun 30, 2023

timtebeek Jul 2, 2023

timtebeek Jul 2, 2023

AlekSimpson Jul 3, 2023

knutwannheden Jul 5, 2023

knutwannheden Jul 5, 2023

knutwannheden Jul 5, 2023

AlekSimpson Jul 5, 2023

AlekSimpson commented Jul 5, 2023

knutwannheden commented Jul 6, 2023

knutwannheden left a comment


		String newValue = replaceY(value);

		return li.withValueSource(newValue).withValue(newValue);

Replace week year (YYYY) with year (yyyy) in date formats #129

Replace week year (YYYY) with year (yyyy) in date formats #129

Conversation

AlekSimpson commented Jun 28, 2023 • edited Loading

What's changed?

What's your motivation?

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

Anyone you would like to review specifically?

Any additional context

Checklist

Choose a reason for hiding this comment

timtebeek left a comment

Choose a reason for hiding this comment

AlekSimpson commented Jun 28, 2023

timtebeek commented Jun 29, 2023

AlekSimpson commented Jun 29, 2023 • edited Loading

timtebeek commented Jun 29, 2023

AlekSimpson commented Jun 29, 2023

timtebeek commented Jun 30, 2023

AlekSimpson commented Jun 30, 2023 • edited Loading

timtebeek commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekSimpson commented Jul 5, 2023

knutwannheden commented Jul 6, 2023

knutwannheden left a comment

Choose a reason for hiding this comment

AlekSimpson commented Jun 28, 2023 •

edited

Loading

AlekSimpson commented Jun 29, 2023 •

edited

Loading

AlekSimpson commented Jun 30, 2023 •

edited

Loading