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

Add ability to set project directory. #1115

Merged
merged 1 commit into from Jul 20, 2023

Conversation

altro3
Copy link
Collaborator

@altro3 altro3 commented Jul 14, 2023

Fixed #1101

@altro3 altro3 requested a review from graemerocher July 14, 2023 04:36
@altro3
Copy link
Collaborator Author

altro3 commented Jul 14, 2023

@graemerocher Hi! Could you review and merge last 3 PR's (#1115, #1112, #1113) ?

After it we can release 4.10.0 - last of 4.x series. Then I move these changes to relese 5.0 and all next changes will be in release 5.0 (except for bug fixes, of course)

@altro3
Copy link
Collaborator Author

altro3 commented Jul 15, 2023

@graemerocher Could you also review this PR and #1113 ?

/**
* Loaded project directory from system properties.
*/
public static final String MICRONAUT_INTERNAL_OPENAPI_PROJECT_DIR = "micronaut.internal.openapi.project.dir";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you override getSupportedOptions() and provide all of these constants as annotation processor options

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I understand, this block do the same

изображение

return projectPath;
}

String projectDir = System.getProperty(MICRONAUT_OPENAPI_PROJECT_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't use system properties but the annotation processor options otherwise the compiler can't detect when input values change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't quite understand what's wrong here. As I understand it, it is necessary to describe the properties of the processor, which must be allowed to be loaded as system variables. They are described in the @SupportedOptions annotation. Now everything has been implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

in general parameters to an annotation processor should never be ready from system properties and should always be read from annotation processor arguments, this particular processor abuses that badly in several places which is less than ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, then how is it correct to write the reading of the arguments? Although I'm not sure if there's much to worry about using system variables. I think it's redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@graemerocher Do I need to write something like this?

context.getOptions().get(MICRONAUT_INTERNAL_OPENAPI_PROJECT_DIR)

Copy link
Collaborator Author

@altro3 altro3 Jul 19, 2023

Choose a reason for hiding this comment

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

It's just that I don't understand. I give kapt the annotation processor argument, but I read it with a system property. That is, it turns out that all arguments are already available as system properties. Or does kapt set not an argument, but a system property?

I tested this solution on a test project - it works correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it needs to be context.getOptions().get(MICRONAUT_INTERNAL_OPENAPI_PROJECT_DIR)

@altro3
Copy link
Collaborator Author

altro3 commented Jul 17, 2023

@graemerocher Can I merge it?

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

@altro3 can you add documentation about this? when should people use it and how to use it

@sdelamo sdelamo added the type: improvement A minor improvement to an existing feature label Jul 17, 2023
@altro3 altro3 force-pushed the project_dir branch 3 times, most recently from 7c3cc58 to c10d8f9 Compare July 17, 2023 11:24
@altro3
Copy link
Collaborator Author

altro3 commented Jul 17, 2023

@sdelamo done

@altro3 altro3 mentioned this pull request Jul 19, 2023
@altro3 altro3 requested a review from graemerocher July 19, 2023 12:17
@altro3 altro3 merged commit 91babbf into micronaut-projects:4.10.x Jul 20, 2023
5 of 6 checks passed
@altro3
Copy link
Collaborator Author

altro3 commented Jul 20, 2023

@graemerocher Can I publish releases 5.0.2 and 4.10.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants