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

Make orchard.java.parser-utils/parse-java-test more easily debuggable #205

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Oct 29, 2023

We used to return nil for invalid .java source code.

Now we throw an exception, with the parsing error diagnostics as part of the ex-data.

Note that this doesn't change user-facing behavior, since all exceptions are caught by default in orchard.java.parser/source-info / orchard.java.parser-next/source-info.

This helps debug faulty .java source code, see e.g. aws/aws-sdk-java#3045

Cheers - V

@vemv vemv requested a review from bbatsov October 29, 2023 20:06
CHANGELOG.md Outdated
@@ -5,6 +5,7 @@
### Changes

* [#202](https://github.com/clojure-emacs/orchard/issues/202): `orchard.inspect`: right-align indices when rendering indexed collections.
* Internal: make `orchard.java.parser-utils/parse-java-test` more easily debuggable.
Copy link
Member

Choose a reason for hiding this comment

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

Such changes don't need a changelog entry. (I guess end users are not supposed to use this function directly)

Copy link
Member Author

Choose a reason for hiding this comment

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

It would SGTM to remove it. My reasoning is that users might be interesed in knowing that something changed, so that any new bugs could be correlated to that change.

Copy link
Member

Choose a reason for hiding this comment

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

I normally never document internal changes unless they can be observed externally somehow (e.g. performance improvement). I think that's just noise for many of the users. At any rate - I don't feel strongly about this, so feel free to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Removing

@bbatsov
Copy link
Member

bbatsov commented Oct 30, 2023

I'm fine with the proposed change.

We used to return nil for invalid .java source code.

Now we throw an exception, with the parsing error diagnostics as part of the ex-data.

Note that this doesn't change user-facing behavior, since all exceptions are caught by default in `orchard.java.parser/source-info` / `orchard.java.parser-next/source-info`.

This helps debug faulty .java source code, see e.g. aws/aws-sdk-java#3045
@vemv vemv merged commit d647842 into master Oct 30, 2023
42 of 70 checks passed
@vemv vemv deleted the javadoc-throw branch October 30, 2023 15:50
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

2 participants