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

feat: start working on yaml support #2428

Merged
merged 29 commits into from
Apr 17, 2024
Merged

feat: start working on yaml support #2428

merged 29 commits into from
Apr 17, 2024

Conversation

defnot001
Copy link
Contributor

Summary

first step in issue #2365

This PR has the grammar and all the required steps to get codegen to run (the generated files are also in this PT). I made this PR so other contributors don't have to do the same thing than I did.

Please leave your suggestions and opinions on the very simple grammar I did so far.

Test Plan

Currently nothing shows that my implementation is actually correct.

@github-actions github-actions bot added the A-Tooling Area: internal tools label Apr 12, 2024
Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 5cce5bf
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/661f86eedd86e8000853e84e
😎 Deploy Preview https://deploy-preview-2428--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100
Accessibility: 97
Best Practices: 100
SEO: 93
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

("*", "ASTERISK"),
("#", "HASH"),
("---", "DOC_START"),
("...", "DOC_END"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add & and << here? (for "anchors": https://support.atlassian.com/bitbucket-cloud/docs/yaml-anchors/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that sounds good to me, I overlooked them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://yaml.org/spec/1.2.2/#anchors-and-aliases searching in this file for override and << gave me no results, do you know why that is different than the atlassian site?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised, I used overrides myself when dealing with Gitlab CI configurations (so it's not only Atlassian who uses them). Looking into that now... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely yaml.org would keep their own spec updated right ..... right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Interesting. See https://yaml.org/spec/1.2.2/ext/changes/ , in section "Changes in version 1.2 (revision 1.2.0) (2009-07-21)".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that would mean that in our quest of supporting the latest (for now) we should leave it out. Although that immediatly makes it less useful since most people use yaml for ci and other infra stuff and there it probably still works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this, I'm not sure about the ideal approach for this. Shared grammar for both specs? and running specialized checks later at the parser & linter levels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes since otherwise we take away most of the reason that people would use this.

@defnot001 defnot001 marked this pull request as draft April 12, 2024 19:22
Copy link

codspeed-hq bot commented Apr 13, 2024

CodSpeed Performance Report

Merging #2428 will not alter performance

Comparing defnot001:yaml (5cce5bf) with main (c9a9001)

Summary

✅ 93 untouched benchmarks

crates/biome_yaml_factory/Cargo.toml Show resolved Hide resolved
crates/biome_yaml_factory/Cargo.toml Outdated Show resolved Hide resolved
crates/biome_yaml_syntax/Cargo.toml Show resolved Hide resolved
crates/biome_yaml_syntax/Cargo.toml Outdated Show resolved Hide resolved
@ematipico
Copy link
Member

@defnot001 can we make the PR ready and merge it?

It's ok if there's still nothing, but having the crates in place will unblock everyone

@ematipico ematipico marked this pull request as ready for review April 16, 2024 08:40
@ematipico
Copy link
Member

@defnot001 I just pushed a couple of commits to fix the CI failure. I think the PR is ready to be merged.

@ematipico
Copy link
Member

@defnot001 I think you have to run just f and you should be good with CI

@defnot001
Copy link
Contributor Author

@defnot001 I think you have to run just f and you should be good with CI

perfect, just did that thanks :)

@ematipico ematipico merged commit c9a9001 into biomejs:main Apr 17, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tooling Area: internal tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants