clojure-emacs
Make orchard.namespace/classpath-namespaces resilient to faulty ns declarations
Closes #158
Before submitting a PR make sure the following things have been done:
- [x] The commits are consistent with our contribution guidelines
- [x] You've added tests to cover your change(s)
- [x] All tests are passing
- [x] The new code is not generating reflection warnings
- [x] You've updated the changelog (if adding/changing user-visible functionality)
Perhaps we were intentionally encouraging that namespaces would be correct at every moment - it's part of CIDER's approach to have loadable code that can be run (and not only parsed).
Looking at the tests for read-namespace makes me think it's built to namespaces that cannot be read:
https://github.com/clojure-emacs/orchard/blob/c0e24f71292f4fed8d4d8cf854a99e4a80cf6ee6/test/orchard/namespace_test.clj#L53-L61
But it's always possible that some 3rd party library could accidentally include some faulty unused namespace, which isn't something users can easily fix. So it's more friendly to give users something usable, than not.
I ran into two cases on two different project in the past week were this assumption broke. It was faulty code under our control but we didn't notice what was causing the issue or where to look for an error.
In addition to this change I think it makes sense to be more defensive in cider-nrepl and catch exceptions thrown in the stacktrace handling and warn the user about what's wrong. Let me know if you think this is a good idea and I could take a look at that as a follow-up.
Thanks for the insights!
In addition to this chance I think it makes sense to be more defensive in cider-nrepl
I also think so, and would prefer that we think that we handle all missing edge cases in a single PR - else it's just "git churn" that we're creating (i.e. make a change, then undo it to do something more comprehensive).
I also think so, and would prefer that we think that we handle all missing edge cases in a single PR - else it's just "git churn" that we're creating (i.e. make a change, then undo it to do something more comprehensive).
I do think this change is good as it improves things for an issue we've experienced and I don't think I'd undo it after adding catching to cider-nrepl. I think it's also in line with the current behaviour of returning nil for namespaces that cannot be read. Those improvements will be also be a larger change with a bigger surface area. Also I can't say when I'd get around to it or if the end result will be something I'd be confident in proposing without introducing breakage.
Delaying this PR to wait for the final comprehensive fix reminds me of: https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good
It's pretty safe to assume that most OSS maintainers are already familiar with various truisms (and their applicability to specific scenarios).
So, are we going to do something with this PR or not? I'm favor of just merging it, given that a comprehensive fix is unlikely to happen any time soon.
Good. I'm planning a new CIDER release (perhaps as soon as this week) and it'd be nice if we manage to squeeze in recent Orchard updates there.