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

Replace Locale constructor by Locale builder (#658 / #662) #662

Merged
merged 5 commits into from Sep 22, 2022

Conversation

Bukama
Copy link
Member

@Bukama Bukama commented Aug 30, 2022

Proposed commit message:

Replace Locale constructor by Locale builder (#658 / #662)

The Locale constructor gets deprecated in newer Java version.
Therefore we replace it by the Locale builder.

closed: #658
PR: #662

linked issue: #658

@Bukama Bukama changed the title Replace Locale constructor by Locale builder (#658 / #) Replace Locale constructor by Locale builder (#658 / #662) Aug 30, 2022
@Bukama
Copy link
Member Author

Bukama commented Aug 30, 2022

Made a new branch/PR with clean base
Difference from last

  • Remove PioneerUtils from Democode
  • Added PioneerUtils to extension class
  • Added test with invalid BCP 47 variant

Copy link
Member

@Michael1993 Michael1993 left a comment

Choose a reason for hiding this comment

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

Please fix the build 😇

@Bukama Bukama added the full-build Triggers full build suite on PR label Aug 31, 2022
@Bukama
Copy link
Member Author

Bukama commented Aug 31, 2022

Adding merge-ready for full build

Copy link
Member

@Michael1993 Michael1993 left a comment

Choose a reason for hiding this comment

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

I think the documentation might need an update to reflect these changes.

@Bukama
Copy link
Member Author

Bukama commented Sep 4, 2022

I think the documentation might need an update to reflect these changes.

What do you miss? The examples in DefaultLocaleTimezoneExtensionDemo.java are updated and in the other parts of the documentation I don't see the need for an update.

@beatngu13
Copy link
Member

@Bukama Michael is right, I have overlooked this, too. For example:

* <p>The {@link java.util.Locale} to set as the default locale can be
* configured in several ways:</p>
*
* <ul>
* <li>using a {@link java.util.Locale#forLanguageTag(String) language tag}</li>
* <li>using a {@link java.util.Locale#Locale(String) language}</li>
* <li>using a {@link java.util.Locale#Locale(String, String) language and a county}</li>
* <li>using a {@link java.util.Locale#Locale(String, String, String) language, a county, and a variant}</li>
* </ul>

Here we refer to the deprecated constructors, we no longer use under the hood. It can be seen as an implementation detail, but I believe it is important. Maybe refer to Locale.Builder?

Builder is used to build instances of Locale from values configured by the setters. Unlike the Locale constructors, the Builder checks if a value configured by a setter satisfies the syntax requirements defined by the Locale class. A Locale object created by a Builder is well-formed and can be transformed to a well-formed IETF BCP 47 language tag without losing information.


We should also note that once this is merged, main is essentially on 2.0.0-SNAPSHOT. We can longer create a 1.x.y (hotfix) release, at least not with our current pipelines.

@Bukama
Copy link
Member Author

Bukama commented Sep 6, 2022

Ah thanks for the hint, with documentation I checked the documentation subpage :D

@Bukama
Copy link
Member Author

Bukama commented Sep 6, 2022

Docs updated.

We should also note that once this is merged, main is essentially on 2.0.0-SNAPSHOT. We can longer create a 1.x.y (hotfix) release, at least not with our current pipelines.

This is not a thing of this PR. The changes in this PR are totally fine with the 1.x versions as it's still Java 8. The fact that the extension behaves a little bit different is no argument for a major change. In my eyes it's more a bugfix, as invalid variants are not allowed anymore.

If the want/need a way to create a 1.x release after we have set up main for 2.x one has to create such a Github workflow. But again, this is not part of this issue/PR.

@beatngu13
Copy link
Member

This is not a thing of this PR. The changes in this PR are totally fine with the 1.x versions as it's still Java 8. The fact that the extension behaves a little bit different is no argument for a major change. In my eyes it's more a bugfix, as invalid variants are not allowed anymore.

We went down this path with #479 (comment), which gave us #623. 😉 Not sure if we can or want to "sell" this change as a bugfix. Just as we did in our own tests, users may create Locales that are invalid IETF BCP 47 strings. This would be a breaking change for them.

If the want/need a way to create a 1.x release after we have set up main for 2.x one has to create such a Github workflow. But again, this is not part of this issue/PR.

Alternatively, we could also start integrating 2.x changes into a dedicated branch. But yeah, here is indeed the wrong place to discuss this.

@Bukama
Copy link
Member Author

Bukama commented Sep 8, 2022

Well it's the same discussion in the linked issues/PR: Is a bug fix, which does not accept past inputs a breaking change and therefor a major release? For me - absolutely not. The only thing I can get in mind is that you rewrite a whole functionality because you can't fix the bug, but then I would not call that a bugfix, but a rewrite of the functionality where the bug was only the point where the stone got rolling.

So what about a mid was? We meet inbetween and call this change worth a minor release?

Nevertheless I add the block label if you decide to still think it's not mergeable before 2.0

@nipafx
Copy link
Member

nipafx commented Sep 14, 2022

If be7295d gets merged in #651, it needs to be reverted in this PR.

@nipafx nipafx added 🚦 status: ready Something that is ready to be closed/merged but waiting for something and removed 🚦 status: blocked labels Sep 14, 2022
@beatngu13 beatngu13 changed the base branch from main to lab/2.0 September 22, 2022 22:07
@beatngu13 beatngu13 merged commit 0e37984 into junit-pioneer:lab/2.0 Sep 22, 2022
@Bukama Bukama deleted the bishue658-locale-secondtry branch September 23, 2022 16:58
beatngu13 pushed a commit that referenced this pull request Sep 28, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Oct 6, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Oct 9, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
nipafx pushed a commit that referenced this pull request Nov 10, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Nov 17, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Nov 26, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Nov 26, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
beatngu13 pushed a commit that referenced this pull request Dec 1, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
nipafx pushed a commit that referenced this pull request Dec 20, 2022
The Locale constructor is deprecated as of Java 19. Therefore, we
replace it by the `Locale.Builder`, which now does syntax checking.

Closes: #658
PR: #662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR 🚦 status: ready Something that is ready to be closed/merged but waiting for something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants