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

fix for #7640 (formating related issue in javadoc) #7641

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

naren2605
Copy link
Contributor

@naren2605 naren2605 commented Aug 6, 2024

fixes issue mentioned here #7640 :

  • @return is always treated as block tag , since java 16 return can be both inline and block tag as mentioned in the the spec
  • this also happens with any inline tag which is not present in list mentioned here

fix:

  • fix provided to check inline tags with inline tag prefix instead of checking with a list , as this list can grow and would impact other inline tags which are not there in the list

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) JavaDoc [ci] enable java/javadoc tests and build-javadoc target labels Aug 6, 2024
@mbien mbien linked an issue Aug 6, 2024 that may be closed by this pull request
@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Aug 6, 2024
@lahodaj
Copy link
Contributor

lahodaj commented Aug 6, 2024

I am sorry, but it seems this patch contains way too many "drive-by" changes to allow a reasonable review. I probably could accept having constants instead of hardcoded number, but changing lastWSOffset to lastWhiteSpaceOffset does not seem very useful to me. I would suggest to cleanup the patch, removing as many of drive-by changes (changes that are not necessary for the functionality of the patch).

@lahodaj lahodaj requested a review from dbalek August 6, 2024 12:55
@mbien
Copy link
Member

mbien commented Aug 6, 2024

@lahodaj its two commits, the first refactors, the second fixes the issue. We do allow the two commit approach, otherwise nobody would ever refactor anything ;)

But I agree that the renaming could be probably reduced a bit.

@ebarboni ebarboni added this to the NB24 milestone Aug 7, 2024
@naren2605
Copy link
Contributor Author

naren2605 commented Aug 8, 2024

I am sorry, but it seems this patch contains way too many "drive-by" changes to allow a reasonable review. I probably could accept having constants instead of hardcoded number, but changing lastWSOffset to lastWhiteSpaceOffset does not seem very useful to me. I would suggest to cleanup the patch, removing as many of drive-by changes (changes that are not necessary for the functionality of the patch).

hi @lahodaj , @mbien thanks for taking a look into pr , i have added minor refactoring changes to improve readability of code in the same method where fix related changes exist(while i was debugging the issue), just following this clean code principle. as @mbien mentioned i have added refactoring changes in one commit(where tests are passing before adding fix) and fix as head commit(with new tests). i would like to summarise refactoring changes as below. take a look and let me know which and all refactoring changes you see doesn't add value, will revert them accordingly.

  1. introduced variables like ACTION_TYPE_ADD_BLANK_LINE and STATE_INITIAL_TEXT as constants to remove code smell - magic numbers (as you are ok with this and @mbien was suggesting to add enums, will enumize these)
  2. variable like currNWSPos, lastNWSPos , currWSPos, lastWSPos, NWS and WS here was bit cryptic i have renamed these variables accordingly to improve readability(will revert these changes as you suggested these doesn't seem to be adding value)
  3. renamed variable names from toAdd to marker , marker noun suited better as we are adding marked position and action to LinkedList<Pair<Integer, Integer>> marks = new LinkedList<Pair<Integer, Integer>>() , renamed variablecheckOffset verb to markedOffset noun as we are retrieving markedOffset from marks list
  4. marked few variables like offset as final , as current method is ~700 lines long and multiple variables and states are mutating, just marked some variables like these to final which are used as readonly values with the intent to improve readability.

let me know @lahodaj if you don't see value add on point's 3,4 will go ahead and revert them.

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

looks good to me, added some minor comments mostly about formatting issues

please sqush into two commits, one with the refactoring, the other with the fix and force push.

Once done we can run CI and get a dev-build from it for a manual smoke test.

… of defined tags as this list can grow + testcases

addressing review comment - bumping java source version to use text blocks
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

looks good to me - thanks. A tested this a little bit and all inline tags are now left untouched by the formatter.

Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

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

Looks fine. All formating tests are passing.

Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Seems OK.

@naren2605
Copy link
Contributor Author

naren2605 commented Aug 28, 2024

@mbien / @lahodaj when are we targeting this pr to be merged?

@mbien
Copy link
Member

mbien commented Aug 28, 2024

three approvals, java tests ran and are green, commit header/email address looks valid, fix and refactoring is kept separate -> merging

@naren2605 thanks and congrats on your first NetBeans contribution ;)

@mbien mbien merged commit f76bd96 into apache:master Aug 28, 2024
36 checks passed
@naren2605
Copy link
Contributor Author

thank you @mbien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) JavaDoc [ci] enable java/javadoc tests and build-javadoc target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting issue with {@return foo}
5 participants