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

fix: generate unescaped boxplot calculated field aliases #9284

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

n8agrin
Copy link
Contributor

@n8agrin n8agrin commented Mar 15, 2024

PR Description

This change ensures all field aliases are unescaped when generated. Before this change if a field was referenced like "field": "one\\.two" the boxplot code would generate calculated fields like "as": "lower_box_one\\.two". This breaks downstream in the compiled Vega code which expects aliases to not include escape sequences.

As far as I could tell there is no issue open for this. The contributing.md doc explicitly calls out it is not necessary to open one, but I'm happy to do so if it helps.

n8agrin added 2 commits March 15, 2024 09:29
Fields like foo.bar are transformed into boxplot parts by creating aliased calculated fields like:

lower_box_foo\\.bar

Aliases don't accept escaped field names. This strips the escaped characters.
@n8agrin n8agrin requested a review from a team as a code owner March 15, 2024 17:05
test/compositemark/boxplot.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@n8agrin
Copy link
Contributor Author

n8agrin commented Mar 15, 2024

@domoritz thanks for the quick 👍. Do you know if this is expected to ever resolve?
image

@domoritz
Copy link
Member

It won't automatically. You either need to figure out how to run the particular job on your fork (we can't do that but I think it may work if you set the personal access token as described in https://github.com/vega/vega-lite/blob/main/CONTRIBUTING.md#contributing but I haven't been able to test this in a while). Otherwise I need to make a branch for you in the repo so we can run the checks.

@n8agrin
Copy link
Contributor Author

n8agrin commented Mar 15, 2024

@domoritz got it thanks! Looks like everything has resolve. Appreciate your help!

@domoritz
Copy link
Member

I made a clone in #9285

@domoritz
Copy link
Member

Yeah, once I made a clone, the CI runs and shows up. I'd really like to figure out how we can better support PRs from external contributors.

@n8agrin
Copy link
Contributor Author

n8agrin commented Mar 15, 2024

@domoritz sounds good. I'm fine with either merging, I'll leave it up to you.

@domoritz
Copy link
Member

Thank you

@domoritz domoritz merged commit 44fbceb into vega:main Mar 16, 2024
17 checks passed
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