Skip to content

Conversation

@olafurpg
Copy link
Contributor

@olafurpg olafurpg commented Sep 6, 2021

Previously, lsif-java index would occasionally return a successful
exit code even if it didn't produce a dump.lsif file. Now, it should
return the exit code 1 instead in these scenarios.

Previously, `lsif-java index` would occasionally return a successful
exit code even if it didn't produce a `dump.lsif` file. Now, it should
return the exit code 1 instead in these scenarios.
@olafurpg olafurpg requested a review from Strum355 September 6, 2021 10:32
@olafurpg olafurpg self-assigned this Sep 6, 2021
Comment on lines +174 to 178
}
CommandResult(0, Nil)
}
CommandResult(0, Nil)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Im confused about this, does not returning an explicit CommandResult result in the exit code being set to non-zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CommandResult on line 162 was previously discarded because the if/else wasn't the last statement

Copy link
Contributor

Choose a reason for hiding this comment

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

oh so thats a thing in scala 🤔 TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are compiler flags to turn that into an error but I don't like using it because there are too many false positives (lots of side-effecting methods return non-Unit values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably messed up because I've been writing too much Java/Go lately with early returns. I don't remember being hit by this in the past, it's usually a non-issue in my experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

It mightve been introduced by me here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, hehe looks like that indeed 😜

@olafurpg olafurpg merged commit 647def5 into sourcegraph:main Sep 6, 2021
@olafurpg olafurpg deleted the exit-code branch September 6, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants