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

Flatten Yaml Exception Test #431

Merged

Conversation

russellbanks
Copy link
Contributor

#27 (comment)

Fully multiplatform tests can't be nested due to limitations in Javascript. This flattens YamlExceptionTest to describe itself as before but without any nesting.

If you're okay with these changes, I can do them for the other tests, with some refactactoring (splitting into smaller parts) to address #27

@charleskorn
Copy link
Owner

charleskorn commented Jun 3, 2023

Thanks for the PR @russelbanks. I'd prefer to leave things as-is for the moment, kaml currently doesn't support JS as a target and there are no immediate plans to.

@russellbanks
Copy link
Contributor Author

If we're able to get through #232 and potentially use the multiplatform SnakeYaml described there eventually, JS would be a target. I'd love to see Kaml be multiplatform

@charleskorn
Copy link
Owner

I'd love to see Kaml be multiplatform

Me too :) But I don't want to go to all the effort of refactoring the tests unless that becomes a reality.

@krzema12
Copy link
Contributor

krzema12 commented Jul 7, 2023

@charleskorn I think we're waiting for your go-ahead to flatten the rest of the tests, this PR being a pilot. Could you take a look?

@charleskorn charleskorn merged commit cdbfadc into charleskorn:main Jul 9, 2023
@krzema12
Copy link
Contributor

krzema12 commented Jul 9, 2023

@charleskorn thanks for merging! @russellbanks are you fine with migrating the rest of the tests?

@russellbanks russellbanks deleted the flatten-yaml-exception-test branch July 9, 2023 07:58
@russellbanks
Copy link
Contributor Author

@russellbanks are you fine with migrating the rest of the tests?

Of course! More PRs incoming!

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.

3 participants