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

easier extending custom collections #467

Merged
merged 11 commits into from May 6, 2023

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Apr 21, 2023

Fix: #457

Still todo:

  • tests for custom collection tags
  • update docs with examples of extending YAMLMap and YAMLSeq
  • catch the error when you have a tag object that specifies collection: 'seq' but gets a map token, and vice versa. (In that case, I imagine it should raise a warning and fall back to YAMLMap or YAMLSeq as appropriate?)

@isaacs isaacs mentioned this pull request Apr 21, 2023
@isaacs isaacs marked this pull request as ready for review April 21, 2023 23:34
Copy link
Owner

@eemeli eemeli left a 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?

docs/06_custom_tags.md Show resolved Hide resolved
src/compose/compose-collection.ts Outdated Show resolved Hide resolved
src/compose/compose-collection.ts Outdated Show resolved Hide resolved
src/compose/compose-collection.ts Outdated Show resolved Hide resolved
src/compose/compose-collection.ts Outdated Show resolved Hide resolved
src/compose/resolve-flow-collection.ts Outdated Show resolved Hide resolved
src/schema/common/map.ts Show resolved Hide resolved
src/schema/yaml-1.1/timestamp.ts Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor Author

isaacs commented Apr 29, 2023

Sure, I can put those methods back. They could just call into Class.from() I guess?

@eemeli
Copy link
Owner

eemeli commented Apr 29, 2023

Sure, I can put those methods back. They could just call into Class.from() I guess?

Yeah, that should work.

@isaacs isaacs force-pushed the isaacs/easier-extending-custom-collections branch from 3cea7e7 to bfca91f Compare April 29, 2023 20:39
@isaacs
Copy link
Contributor Author

isaacs commented Apr 29, 2023

Ok, pushed with updates noted. Also reworded commits to conform with the semantic-commit style here. PTAL, thanks :)

@isaacs isaacs force-pushed the isaacs/easier-extending-custom-collections branch from bfca91f to b963fc7 Compare April 29, 2023 20:46
src/schema/types.ts Outdated Show resolved Hide resolved
- remove unnecessary check for undeterminable expType
- lookup tag by both tagName and collection type
- clearer NodeClass typecast
- specify schema param to NodeClass constructor
@isaacs
Copy link
Contributor Author

isaacs commented May 2, 2023

Updated with the comments above.

@isaacs
Copy link
Contributor Author

isaacs commented May 2, 2023

Looks like test failure is due to lacking auth secrets.

Copy link
Owner

@eemeli eemeli left a 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.

src/nodes/YAMLSeq.ts Outdated Show resolved Hide resolved
@eemeli eemeli merged commit b741332 into eemeli:main May 6, 2023
13 checks passed
@eemeli
Copy link
Owner

eemeli commented May 6, 2023

🎉 This is now included in v2.3.0-5, available on npm as yaml@next.

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 yaml-types release out for that and update the docs on custom tags/types.

@isaacs
Copy link
Contributor Author

isaacs commented May 7, 2023

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 :)

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.

Issues with custom tags
2 participants