-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
easier extending custom collections #467
easier extending custom collections #467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, but there are a couple of places where I'm concerned about introducing breaking changes, in particular dropping the createNode
values from the map
and seq
collection tags. Could those be avoided?
Sure, I can put those methods back. They could just call into Class.from() I guess? |
Yeah, that should work. |
3cea7e7
to
bfca91f
Compare
Ok, pushed with updates noted. Also reworded commits to conform with the semantic-commit style here. PTAL, thanks :) |
Prevents circular module reference
Updates from PR review
bfca91f
to
b963fc7
Compare
- remove unnecessary check for undeterminable expType - lookup tag by both tagName and collection type - clearer NodeClass typecast - specify schema param to NodeClass constructor
Updated with the comments above. |
Looks like test failure is due to lacking auth secrets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the remaining lint errors on your branch; it was easier that way.
Having tested this locally, it looks good! I'm pretty sure we've managed to get rid of the breaking changes. The types docs will need some iterating, but that's really a separate change.
There's one inline fix below that's hopefully the last before this can be merged.
🎉 This is now included in v2.3.0-5, available on npm as I think I'll let this stay in prerelease for a week or two still before releasing v2.3.0, in case any issues are found with either the TS changes or this. Would be good to get a proper |
Cool, I'll play with it tonight, and see if I can send a or to add all of tap-yaml's types to yaml-types. Hopefully can put that repo to bed once and for all :) |
Fix: #457
Still todo:
collection: 'seq'
but gets amap
token, and vice versa. (In that case, I imagine it should raise a warning and fall back toYAMLMap
orYAMLSeq
as appropriate?)