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

Minor fixes #9596

Closed
wants to merge 3 commits into from
Closed

Minor fixes #9596

wants to merge 3 commits into from

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Jul 23, 2023

  1. Javadoc lint fixes
  2. Checkstyle fixes
  3. Some typos fixes
  4. Fixed some tests for different system locale
  5. Fixed minimal java release for inject-test JavaParser
  6. Added '--add-opens java.base/java.lang=ALL-UNNAMED' for correct work SystemLambda and '--add-opens jdk.compiler/com.sun.tools.javac.lang=ALL-UNNAMED' for kotlin in tests
  7. Fixed DefaultFileSystemResourceLoader normalize method for windows

@sdelamo sdelamo self-assigned this Jul 28, 2023
@altro3 altro3 force-pushed the minor_fixes_4.0 branch 2 times, most recently from d0258ce to 74e81f9 Compare July 30, 2023 08:08
* @author graemerocher
* @since 1.0
*/
public interface ParametrizedProvider<T> extends Provider<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a breaking change we can't remove it even if it is not used until next major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@graemerocher And how to be? Will this class be present in all releases up to version 5.0 now?

As far as I understand, you forgot to remove it when removed the ParametrizedBeanFactory class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway I rolled back this change

@altro3 altro3 force-pushed the minor_fixes_4.0 branch 2 times, most recently from ba448a0 to bb4c76b Compare August 3, 2023 11:44
@altro3
Copy link
Contributor Author

altro3 commented Aug 19, 2023

@sdelamo @dstepanov Hi! Could you review this changes?

@@ -45,7 +45,7 @@ public enum ExecutorType {
WORK_STEALING,

/**
* @see java.util.concurrent.Executors#newThreadPerTaskExecutor()
* @see io.micronaut.scheduling.LoomSupport#newThreadPerTaskExecutor(java.util.concurrent.ThreadFactory)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yawkat is this change in the javadoc correct?

Copy link
Member

Choose a reason for hiding this comment

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

no. the old one is correct. i assume it triggers a lint failure because it points to a method that is not available on java 17, but that's fine imo.

Copy link
Contributor Author

@altro3 altro3 Aug 21, 2023

Choose a reason for hiding this comment

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

@yawkat hm... yes, it's true. But why it's Ok if we use java 17 and java 17 doesn't have this method in JDK ?

Copy link
Member

Choose a reason for hiding this comment

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

this setting does not work on java 17 anyway. only on jdks where this method is available does it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if there are such JDKs? ok i will revert this change

@@ -129,6 +129,9 @@ private static String normalize(String path) {
}
if (path.startsWith("file:")) {
path = path.substring(5);
if (path.startsWith("//")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? can you add a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem with pathes in windows.... Path like this //D:/Wok/.... is wrong

@@ -36,7 +36,7 @@
public sealed interface MultiObjectBody extends HttpBody permits ImmediateMultiObjectBody, ImmediateSingleObjectBody, StreamingMultiObjectBody {
/**
* Coerce this value to an {@link InputStream}. This implements
* {@link io.micronaut.http.server.netty.binders.InputStreamBodyBinder}. Requires the objects
* io.micronaut.http.server.netty.binders.NettyInputStreamBodyBinder. Requires the objects
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove the @link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... this is maybe a byg in IDEA. I see now this picture:
изображение
But I also don't understand what is wrong...

Reverted

inject/build.gradle Outdated Show resolved Hide resolved
@@ -167,7 +167,7 @@ public JsonNode getCompletedValue() {
}

@Override
public void writeStartArray() throws IOException {
public void writeStartArray() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why removing the throws. Does not throw IOException anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't need to write exceptions from interface if your implementation don't throw them

runtime/build.gradle Outdated Show resolved Hide resolved
@altro3 altro3 force-pushed the minor_fixes_4.0 branch 2 times, most recently from ef58fca to 8ac9766 Compare August 21, 2023 14:29
@altro3 altro3 changed the base branch from 4.0.x to 4.1.x August 21, 2023 14:29
2. Checkstyle fixes
3. Some typos fixes
4. Fixed some tests for different system locale
5. Fixed minimal java release for inject-test JavaParser
6. Added '--add-opens java.base/java.lang=ALL-UNNAMED' for correct work SystemLambda and '--add-opens jdk.compiler/com.sun.tools.javac.lang=ALL-UNNAMED' for kotlin in tests
7. Fixed DefaultFileSystemResourceLoader normalize method for windows
@altro3 altro3 requested review from yawkat and sdelamo August 25, 2023 04:17
sdelamo added a commit that referenced this pull request Aug 31, 2023
cherry picked javadocs improvements from @altro3 in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
Cherry pick parameterized logging improvements from @altro3 in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
Cherry picked @altro3 changes to SourceVersion in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
cherry picked javadocs improvements from @altro3 in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
Cherry picked @altro3 changes to SourceVersion in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
Cherry pick parameterized logging improvements from @altro3 in #9596
sdelamo added a commit that referenced this pull request Aug 31, 2023
Cherry pick changes from @altro3 in #9596

> without it tests doesn't work on Windows and I see exception with SystemLambda
sdelamo added a commit that referenced this pull request Aug 31, 2023
* build: open java.lang

Cherry pick changes from @altro3 in #9596

> without it tests doesn't work on Windows and I see exception with SystemLambda

* Update build.gradle
@sdelamo sdelamo added this to the 4.2.0 milestone Nov 10, 2023
sdelamo added a commit that referenced this pull request Nov 10, 2023
rename variable to avoid name shadowing of outer `result`

Originally suggested by @altro3 here: #9596
sdelamo added a commit that referenced this pull request Nov 10, 2023
rename variable to avoid name shadowing of outer `result`

Originally suggested by @altro3 here: #9596
@sdelamo
Copy link
Collaborator

sdelamo commented Nov 10, 2023

Closing this as I think we have incorporated almost every suggestion. @altro3 please reopen separate prs if you think we need them.

@sdelamo sdelamo closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants