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

Use parser/safe-zloc-of-string #1092

Merged
merged 3 commits into from Jun 30, 2022
Merged

Conversation

mainej
Copy link
Collaborator

@mainej mainej commented Jun 30, 2022

Converts all usages of parser/zloc-of-string to parser/safe-zloc-of-string. Instead of letting exceptions short-circuit, we short-circuit on nils. This keeps stack traces out of the logs.

This patch also improves the performance of call hierarchies by parsing less. This was just an opportunistic refactoring—while looking at the parsing done in call-hierarchy I noticed were doing it a lot more frequently than necessary. There's more performance work that can be done here. See comments in call_hierarchy.clj.

  • I created an issue to discuss the problem I am trying to solve or an open issue already exists.
  • I added a new entry to CHANGELOG.md
  • I updated documentation if applicable (docs folder)

Converts all usages of `parser/zloc-of-string` to
`parser/safe-zloc-of-string`. Instead of letting exceptions
short-circuit, we short-circuit on nils. This keeps stack traces out of
the logs.

This patch also improves the performance of call hierarchies by parsing
less. There's more performance work that can be done here. See comments
in call_hierarchy.clj.
Comment on lines 30 to 31
;; TODO: tests pass if this branch is removed. Why can't we use
;; the else branch for project files?
Copy link
Member

@ericdallo ericdallo Jun 30, 2022

Choose a reason for hiding this comment

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

Good question, I can't recall why I added that at the time, but if we can always get the namespace reliable and fallback for filenames when not available, seems already good enough, so the else branch makes sense.
Also, parsing the loc just to find the namespace name sounds too much

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the parsing was added all the way back in bc39d0e. It was introduced before we started using ns, which was added in 2e154fa. I think everything could have been switched to ns at that time. I'll make the switch now, keeping the filename fallback. 🎉

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense! Nice

Copy link
Member

@ericdallo ericdallo left a comment

Choose a reason for hiding this comment

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

LGTM

@ericdallo ericdallo merged commit 54b4251 into master Jun 30, 2022
@ericdallo ericdallo deleted the minimize-parse-exceptions-in-logs branch June 30, 2022 22:45
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