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

docs: annotate properly settings and refer to autodoc from the README files #4084

Merged

Conversation

segoranov
Copy link
Contributor

@segoranov segoranov commented Apr 4, 2024

What this PR changes/adds

  • Adds missing description and default values on config parameters annotated with @Setting.
  • Adds reference to generated autodoc and removes the configuration parameters description from the READMEs.
  • Use the constants for default values in @Setting

Why it does that

  • To avoid outdated/inconsistent documentation of configuration parameters and double maintenance.
  • To keep the settings config consistent throughout the project.

Further notes

Other configuration params not reported in the original issue were also corrected as described.

Linked Issue(s)

Closes #3986

… files (eclipse-edc#3986)

* avoid outdatedness of the documentation of config params in README files
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

/**
* Provides support for reading data from an HTTP endpoint and sending data to an HTTP endpoint.
*/
@Provides(HttpRequestParamsProvider.class)
@Extension(value = DataPlaneHttpExtension.NAME)
public class DataPlaneHttpExtension implements ServiceExtension {
public static final String NAME = "Data Plane HTTP";
private static final int DEFAULT_PART_SIZE = 5;
private static final String DEFAULT_PARTITION_SIZE = "5";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't chnage the type and parse it below. The annotation type system is more restrictive, so create another constant that converts the typed int value to a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think it's possible to create another constant which converts the typed int value to string like this:
private static final String DEFAULT_PARTITION_SIZE_STRING = String.valueOf(DEFAULT_PARTITION_SIZE);
because in @Setting we have to use compile-time constant.

The other option would be to hardcode it like private static final String DEFAULT_PARTITION_SIZE_STRING = "5" or directly put "5" in the @Setting but like this we still have double-maintenance problem.

(The usage of string and parsing to int I saw in CoreDefaultServicesExtension for example).

Copy link
Member

Choose a reason for hiding this comment

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

you can do DEFAULT_PART_SIZE + "" to convert to string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Should I also go and fix similar doc/config/parse stuff in CoreDefaultServicesExtension and other places throughout the project, to make it uniform? Or rather new issue(s) should be created, to avoid broad-scoped PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK to include it here if the changes are small.

@segoranov segoranov requested a review from jimmarino April 4, 2024 14:25
@ndr-brt
Copy link
Member

ndr-brt commented Apr 5, 2024

is this ready for review or still a draft?

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.68%. Comparing base (7f20ba5) to head (1662f39).
Report is 208 commits behind head on main.

Files Patch % Lines
...ipse/edc/connector/core/CoreServicesExtension.java 50.00% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4084      +/-   ##
==========================================
+ Coverage   71.74%   74.68%   +2.93%     
==========================================
  Files         919      979      +60     
  Lines       18457    20087    +1630     
  Branches     1037     1131      +94     
==========================================
+ Hits        13242    15002    +1760     
+ Misses       4756     4591     -165     
- Partials      459      494      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@segoranov
Copy link
Contributor Author

is this ready for review or still a draft?

It's ready, two checks have failed though, one is for lack of PR label (i looked for a way to add documentation but probably i do not have permissions to do it). The other is about some outdated dependencies file, not sure atm how to proceed there. Should i mark it as ready for review or first fix the failing checks?

@ndr-brt ndr-brt added the documentation Improvements or additions to documentation label Apr 5, 2024
@ndr-brt
Copy link
Member

ndr-brt commented Apr 5, 2024

is this ready for review or still a draft?

It's ready, two checks have failed though, one is for lack of PR label (i looked for a way to add documentation but probably i do not have permissions to do it). The other is about some outdated dependencies file, not sure atm how to proceed there. Should i mark it as ready for review or first fix the failing checks?

I added the label, regarding the dependencies check, you can copy the output of the job from the github action console log and paste in your DEPENDECIES file, that will solve the problem.
I'd say let's mark it ready for review, so reviewers will understand that they can proceed.

@segoranov segoranov marked this pull request as ready for review April 5, 2024 12:22
@segoranov
Copy link
Contributor Author

All done. Should I squash all of the commits into 1 after approvals before merging or the one who merges the PR will do that?

@ndr-brt
Copy link
Member

ndr-brt commented Apr 9, 2024

it's not necessary because we will squash them at the merge.
Please re-request my review if you desire a new review

@segoranov segoranov requested a review from ndr-brt April 9, 2024 07:27
core/common/connector-core/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-control-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-http/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-public-api/README.md Outdated Show resolved Hide resolved
@segoranov segoranov requested a review from ndr-brt April 9, 2024 11:05
@ndr-brt ndr-brt merged commit afd11ae into eclipse-edc:main Apr 10, 2024
16 checks passed
@ndr-brt
Copy link
Member

ndr-brt commented Apr 10, 2024

thanks

@segoranov segoranov deleted the fix/outdated-settings-documentation branch April 10, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation for edc.transfer.proxy.token... config is wrong
4 participants