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

Amending YAML parser to add support for tags #4934

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

greg-at-moderne
Copy link
Contributor

@greg-at-moderne greg-at-moderne commented Jan 22, 2025

What's changed?

Adding support for tags in YAML, e.g.

AttributeDefinitions: !Dynamo Title

!Dynamo is a tag here.

What's your motivation?

Observed in real-life YAMLs that they sometimes use tags.
Currently they have been accidentally supported in some cases. E.g. for scalars we used to consider !Dynamo Title to be the value.

Anything in particular you'd like reviewers to focus on?

The LST model for YAML has been amended to include two new fields called tag in Scalar and Sequence. Which is not fully backward compatible. E.g. if someone has a recipe which does new Yaml.Scalar(...), the code will no longer compile for 1 extra argument missing.
Not sure how do we provide coverage for these scenarios.

@greg-at-moderne greg-at-moderne added bug Something isn't working parser-yaml labels Jan 22, 2025
@greg-at-moderne greg-at-moderne self-assigned this Jan 22, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/java/org/openrewrite/gradle/RemoveRedundantDependencyVersions.java
    • lines 240-241
  • rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
    • lines 1278-1278

@knutwannheden
Copy link
Contributor

It looks like we haven't added support for tags to Yaml.Mapping. In the globalTags() test case there is one example and the tags ends up in the prefix of the mapping's first entry (see debugger screenshot). Here is the corresponding YAML:

person: !!map
  name: John Doe
  age: 30
image

I think it would probably make sense to support YAML mapping tags at the same time.

Further, I think the !! syntax are implicit global tags. There are also explicit global tags like: !<tag:yaml.org,2002:str> "Hello" (which is semantically equivalent to !!str) and any tag not using the !<> (or !!) syntax is a "local" tag. I wonder if these details should be part of the LST model rather than being implicitly encoded as the tag's "name".

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/java/org/openrewrite/gradle/RemoveRedundantDependencyVersions.java
    • lines 240-241
  • rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
    • lines 1278-1278

@greg-at-moderne
Copy link
Contributor Author

It looks like we haven't added support for tags to Yaml.Mapping

Added in 6344b6a

@greg-at-moderne
Copy link
Contributor Author

tag's "name"

Renamed the field to text in 79f22f7

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-gradle/src/main/java/org/openrewrite/gradle/RemoveRedundantDependencyVersions.java
    • lines 240-241
  • rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
    • lines 1278-1278

@greg-at-moderne
Copy link
Contributor Author

I wonder if these details should be part of the LST model rather than being implicitly encoded as the tag's "name".

Added in adf3fbb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser-yaml
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants