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

Minor syntax improvements #514

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Apr 6, 2022

This PR applies syntax improvements that are the result of Intellij's default code inspections.

Some additional notes

  • Intellij recommends adding type signatures to methods/fields which are public (hence the reason behind a lot of the addition of manual type annotations). I have the suspicion that a lot of these fields should actually be private and not public (likely the reason why the types were not there in the first place). If so let me know
  • Unit type annotation was added to any method that happened to return Unit (i.e. side effecting methods that don't return anything).
  • The various .toSeq conversions were removed since Intellij marked them as pointless (being the same type). I am quite sure this is not going to cause issues but I would double check here just to make sure nothing odd happens
  • the @tailrec annotation doesn't actually change anything in the resulting generated bytecode but it will actually confirm that scalac will correctly translation the recursion into an efficient while loop.
  • There were some cases where either .close() wasn't being called, or it was but the expression wasn't in a try/finally block which would mean if there is an exception (for some reason) then the handle's wouldn't be released. I have fixed these cases.

@@ -78,7 +78,13 @@ class ErrorCollector extends ErrorContext {
.foreach {
case (page, errors) =>
// Load contents of the page
val lines = scala.io.Source.fromFile(page)("UTF-8").getLines().toList
Copy link
Contributor Author

@mdedetrich mdedetrich Apr 6, 2022

Choose a reason for hiding this comment

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

Intellij handily picked up the fact that the handle from source is never actually closed which actually causes a leak hence the change here. There are nicer ways of doing this i.e. using Using but this is only available in Scala 2.13

@mdedetrich mdedetrich force-pushed the syntax-improvements branch 4 times, most recently from b1863a9 to 0371bc6 Compare April 6, 2022 09:41
@mdedetrich
Copy link
Contributor Author

So the latest test run is failing on VarsDirectiveSpec even though it passes fine locally with JDK8 and the previous github actions test run passed without problems (see https://github.com/lightbend/paradox/actions/runs/2101609267).

Maybe rerunning the tests should work? (although not sure why such a test would be flaky)

Copy link
Contributor

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, nice little cleanup, thanks!

@johanandren
Copy link
Contributor

Oh, I missed the test failure, which is ofc not LGTM :)

@mdedetrich
Copy link
Contributor Author

@johanandren Can you rerun the tests? The tests passed in a previous run and locally, I actually cannot replicate this error (or something really weird is going on)

@pvlugter
Copy link
Member

pvlugter commented Apr 6, 2022

Not sure what happened with the tests, rerun is fine.

Copy link
Member

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

👍🏼 nice cleanup

@pvlugter pvlugter merged commit 1a84ca8 into lightbend:master Apr 6, 2022
@mdedetrich mdedetrich deleted the syntax-improvements branch April 7, 2022 07:33
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.

None yet

3 participants