unhandled kwargs throw errors #422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 94.61% 96.17% +1.56%
==========================================
Files 14 14
Lines 4178 4185 +7
==========================================
+ Hits 3953 4025 +72
+ Misses 225 160 -65 ☔ View full report in Codecov by Sentry. |
| function build_tree(::Type{GreenNode}, stream::ParseStream; kws...) | ||
| build_tree(GreenNode{SyntaxHead}, stream; kws...) do h, srcrange, cs | ||
| # we don't need the `filename` and `first_line` kwargs, but some trees do, so we allow them to be passed | ||
| function build_tree(::Type{GreenNode}, stream::ParseStream; filename=nothing, first_line=nothing) |
There was a problem hiding this comment.
It's confusing to know what to do with this part: do we want all build_tree(::Type{TreeType}, ...) methods to need to know about each other for all TreeType? This seems impossible (eg, external packages) which would imply these should just be an error rather than ignored.
But on the other hand making this an error means build_tree() with keywords can't be used generically.
It seems there's two consistent things to do:
- Be generic and ignore unknown keywords (current status quo. not a good user experience, as in MethodError: no method matching
isless(::Int64, ::Nothing)#341) - Throw on unknown keywords (makes
parsealletc couple theTreeTypewith keywords)
This PR takes an awkward middle path between those two options. Which might be ok but seems inconsistent?
There was a problem hiding this comment.
Yeah, I guess if we want a more decoupled approach, we could copy Makie and add a validate_kwargs function which is called on the tree type and the passed-in kwargs (and throws or returns nothing), and is called in parseall or _parse or something like that. Then custom trees which wish to support additional kwargs would need to define a validate_kwargs method to allow them to pass through.
There was a problem hiding this comment.
That sounds promising! So we define validate_parse_args(::Type{GreenNode}; kws...) etc and users with custom trees can overload it?
closes #416
My editor is configured to remove trailing whitespace, which caused some diffs here. Let me know if that's too annoying and I'll undo it.
Locally I get the test failures:
same as in #420, which I think are unrelated.