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

Utility for generating 'providers.md' #297

Merged
merged 15 commits into from
Aug 24, 2022

Conversation

panilya
Copy link
Collaborator

@panilya panilya commented Aug 15, 2022

At that moment, it doesn't support the descriptions. I will add later

@panilya panilya marked this pull request as draft August 15, 2022 12:30
@panilya panilya marked this pull request as ready for review August 15, 2022 12:30
@bodiam bodiam marked this pull request as draft August 15, 2022 12:56
@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2022

Codecov Report

Merging #297 (4ca502d) into master (87a721b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #297   +/-   ##
=========================================
  Coverage     94.86%   94.86%           
- Complexity     1893     1894    +1     
=========================================
  Files           197      197           
  Lines          3797     3797           
  Branches        379      379           
=========================================
  Hits           3602     3602           
  Misses           99       99           
  Partials         96       96           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bodiam
Copy link
Contributor

bodiam commented Aug 15, 2022

That looks very interesting. Nice work, this would be really nice to have to update the documentation with!

@panilya
Copy link
Collaborator Author

panilya commented Aug 16, 2022

How about introducing a file with descriptions of providers providers_description.yml, and getting descriptions of providers from that file when generating providers' docs with ProvidersDocsGenerator?

@snuyanzin
Copy link
Collaborator

Why can't it be done in same javadoc with something like {@description your description here} ?

@panilya
Copy link
Collaborator Author

panilya commented Aug 16, 2022

yes, your idea is better

@panilya
Copy link
Collaborator Author

panilya commented Aug 16, 2022

Writing description in { } inlines description with @since tag in the same line, so it looks kind of unreadable. Do you know to fix it?

@snuyanzin
Copy link
Collaborator

Didn't get what you mean.... could you elaborate please?

I mean something like this

/**
 * @since 0.8.0
 * {@description your
 * multiline
 * description
 * }
 */

@panilya
Copy link
Collaborator Author

panilya commented Aug 16, 2022

I mean this:

image
image

But if this doesn't matter, I will implement this. I apologize for the lack of clarity.

@bodiam
Copy link
Contributor

bodiam commented Aug 16, 2022

No, ideally don't do that (introducing a new file or that syntax with @description.

The javadoc as-is is fine imo, please just use that, and don't introduce something special for this, unless there's a really good reason for that, which at the moment I don't see.

@panilya
Copy link
Collaborator Author

panilya commented Aug 16, 2022

everything looks good to me, what do you think? I think it's ready for merge

Co-authored-by: Sergey Nuyanzin <sergey.nuyanzin@aiven.io>
@panilya panilya marked this pull request as ready for review August 16, 2022 13:20
@snuyanzin
Copy link
Collaborator

Regexp does not work properly
to check run and have a look at record with CPF, column since.
To fix it regexp should be changed to something like "@since\\s+\\d\\.\\d+\\.\\d"
and net.datafaker.script.ProvidersDocsGenerator#matchSinceTag
to

  private String matchSinceTag(String javaDocComment) {
        Matcher comment = pattern.matcher(javaDocComment);

        if (comment.find()) {
            return comment.group().substring("@since".length()).trim();
        }
        return "";
    }

@snuyanzin
Copy link
Collaborator

ok from my point of view it looks more or less ok
there are 2 tiny things

  1. Probably src/test/java/net/datafaker/script/providers.md should be changed to more suitable location
  2. May be it makes sense to make build failing when fakersWithoutSinceTag is not empty however it could be done within a different issue

@bodiam do you have anything to add?

@bodiam
Copy link
Contributor

bodiam commented Aug 17, 2022

I'm following it from the side lines, but I think you've covered everything. I don't mind the output location that much, but I think something in the target directory is probably better, or, if you're feeling confident, just overwrite the actual page in the docs.

@bodiam
Copy link
Contributor

bodiam commented Aug 22, 2022

I'm thinking of making a new release, is there anything open on this PR?

@panilya
Copy link
Collaborator Author

panilya commented Aug 23, 2022

@bodiam Currently, this utility doesn't include descriptions as in the original one (CNPFJ, CPF, and Mountains), so you need manually copy them. And, because of that, it's not recommended to rewrite the original providers.md. If @snuyanzin doesn't have any ideas or suggestions, you can merge this PR

@bodiam bodiam merged commit d053424 into datafaker-net:master Aug 24, 2022
@bodiam
Copy link
Contributor

bodiam commented Aug 24, 2022

@panilya Awesome, I'll fix that. The code is a bit strange at some places, for example the extractTagFromJavadoc. You might think it extracts tags from Javadoc, but it's doesn't. It actually returns comments, and not only that, it also keeps track of files which didn't have a @since field. Also, it doesn't extract things from JavaDoc, it reads files. So, this method should actually be called:

extractJavaodcFromSourceFileAndKeepTrackOfFilesWhichDontHaveASinceField

which is a bit of a mouth full. So, instead, it would probably be better to give this method task: either extract comments form the Javadoc, or keep track of files which don't have a @since field, but not both.

There's really no need to change this, I really appreciate your work on this, but it's just some feedback for in the future, and part of SOLID/SRP(Single Responsibility Pattern), which might be good reading material if you feel like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants