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

Expose maxPaddingWidth in OutputSettings keeping default as 30 #1655

merged 2 commits into from Oct 21, 2021


Copy link

Fixes #1653

In order to get my fork up and going, I updated github actions here. I removed jdk 16, added jdk 18-ea, and moved to zulu distribution as temurin does not have 18-ea.

For this change, I added a changelog, added some clarification to 'memoised' padding, deprecated original padding call by having it call newer one with exposed maxPaddingWidth, skipped the min check with -1, and finally called the new one. In an attempt to better understand 'memoised' logic, I actually added more tests than necessary to the existing one for clarity purposes on how that method behaves.

The overall change though tripped up the japicmp plugin. It kept saying the method had been removed. If I removed the deprecation markings I added, it was happy. So I think that plugin only really works against deprecations from prior version. I tried a few different ways to make that work and finally gave up so I just added the class to ignore for the time being. I believe that is a bug in their plugin and that deprecation is important if anyone is otherwise externally using that method since it directs them on what to update.

One other thing, on my fork, github no longer auto turns on actions so I had to get that working so I hunted for some basic items to get it going. I noticed that jetty-servlet was not updated. That is odd given dependabot usage and the other module from jetty was updated so I bumped it up.

Obviously this has more than what I should have for a new pull request. For purposes of review, please use this. If you do need me to then rebase and only send up direct change portions, no problem, I can rebase as necessary.



Copy link

jhy commented Oct 19, 2021

Thanks Jeremy, this looks good. One thing: I think we should leave the existing StringUtils.padding(width) as not deprecated. And modify the comment so that it mentions it currently defaults to 30 (i.e. not part of the contract). If people are using this method currently and are happy with it, I don't feel that they need to take on the requirement to migrate to a method that allows customization.

Yes, please go ahead and re-base so it only includes the code changes, not the GitHub action runners.

Without it being marked deprecated, we also don't need the ignore line in the POM. BTW, I don't think that's a javacmp bug so much as an interaction with the current configuration. I have set it to ignore methods that are marked deprecated so that I could delete a set of old methods. But I guess introducing a new deprecated method that was then ignored looks like it was unexpectedly removed.

@jhy jhy added this to the 1.15.1 milestone Oct 19, 2021
Copy link
Contributor Author

@jhy Thanks for the review. I have rebased against your current master and then rebased my commits so only the padding is in this now. The original method did note the 30 max so I added a note there if they need more to use the other method. For the other items, I'll save those for a separate set of pull requests later. Thanks.

And added a test that the max level set in OutputSettings is applied.
@jhy jhy merged commit 3bd4d79 into jhy:master Oct 21, 2021
Copy link

jhy commented Oct 21, 2021

Thanks, merged! I made a small change to make sure that the maximum is applied before the memo check, in case one wants a maximum < 20. Also added a testcase for the OutputSettings to make sure those methods are actually rigged up.

Copy link
Contributor Author

hazendaz commented Oct 21, 2021 via email

sakibguy added a commit to sakibguy/jsoup that referenced this pull request Oct 21, 2021
Expose maxPaddingWidth in OutputSettings keeping default as 30 (jhy#1655)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

Please expose maxPaddingWidth in two ways
2 participants