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

Port several types from tap-yaml #3

Merged
merged 4 commits into from
May 19, 2023
Merged

Conversation

isaacs
Copy link
Collaborator

@isaacs isaacs commented May 9, 2023

Happy to split this up into multiple PRs if it's easier to review separately, or send a smaller PR if only some are desired.

This is about 80% of the way to being able to just drop tap-yaml entirely in favor of yaml-types, which would be nice.

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.

Very promising! Happy to consider all of these at once. Did a first pass, but ran out of time before getting to !function and !nullobject. See comments inline.

README.md Show resolved Hide resolved
src/class.ts Outdated Show resolved Hide resolved
src/class.ts Outdated Show resolved Hide resolved
src/class.ts Outdated Show resolved Hide resolved
src/class.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
src/error.ts Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@isaacs isaacs force-pushed the isaacs/tap-yaml-port branch 3 times, most recently from 0f75ceb to 7a10202 Compare May 9, 2023 19:21
@isaacs
Copy link
Collaborator Author

isaacs commented May 9, 2023

Resolved the typos and style stuff etc that you caught, thanks for the review.

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.

Finally got around to this again; sorry for the delays. Two inline comments still to resolve; I think Domains ought to get left out for now. Happy to reconsider later if another use case for them comes up.

@isaacs
Copy link
Collaborator Author

isaacs commented May 17, 2023

Pulled out !domain, and updated !error with the patch I suggested, if that works. (I'm not opinionated about it, if you wanna land it the way you suggested, that works fine too.)

And actually, now that I think about it, since domains are deprecated and not really used much anymore, tap probably shouldn't give them any special treatment, and instead just remove them from the diagnostic objects anyway.

@eemeli
Copy link
Owner

eemeli commented May 18, 2023

It looks like there are a few remaining test & lint errors. Other than those, I think this is good to merge.

I sent you an invite to the repo, which should let the CI tests run automatically.

@isaacs
Copy link
Collaborator Author

isaacs commented May 18, 2023

Sweet, thanks! I'll take a look this afternoon and see if I can't get the CI passing.

@isaacs
Copy link
Collaborator Author

isaacs commented May 19, 2023

Test failure was because ??= was being used to test against a false value, because typeof === 'function' returns false, not null.

Fixed and ready to land, just waiting for CI to complete.

@isaacs isaacs merged commit ba5cb2c into eemeli:main May 19, 2023
@isaacs isaacs deleted the isaacs/tap-yaml-port branch May 19, 2023 19:25
@isaacs
Copy link
Collaborator Author

isaacs commented May 23, 2023

@eemeli merged, but you've got the publishing keys ;)

@eemeli
Copy link
Owner

eemeli commented May 23, 2023

Done! Ended up releasing yaml 2.3.0 first, and added that as a minimum dependency here.

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.

2 participants