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: Add prefix to names in a layer #8663

Closed
wants to merge 2 commits into from
Closed

fix: Add prefix to names in a layer #8663

wants to merge 2 commits into from

Conversation

ChristopherDavisUCI
Copy link
Contributor

This is an attempt to resolve the issues @mattijn raised in #8348 and #8446. (I believe this PR is independent from my other recent PR, #8662.)

I'm very happy to hear comments:

  • Is this approach, making sure there are no duplicate names, the correct approach?
  • Is this the right place to make the change?
  • Is this a reasonable name format (probably not): params.repeaterPrefix + spec.name
  • Is there a more natural way to write this logic in JavaScript?
name: spec.name && params.repeaterPrefix ? params.repeaterPrefix + spec.name : undefined

A test is currently failing because of this line:

expect(normalized.concat[0].layer[0].name).toBe('a');

Is it correct to have both normalized.concat[0].layer[0].name and normalized.concat[1].layer[0].name equal to "a"? This PR would change that, so it would break this test.

Thanks again for any comments!

@ChristopherDavisUCI
Copy link
Contributor Author

Hi @domoritz, thanks for the reviews of my other PRs. I changed this one to "ready for review" also, although please see my many questions in the original comment above.

@domoritz domoritz changed the base branch from next to main February 20, 2023 15:39
@@ -65,6 +65,7 @@ export class CoreNormalizer extends SpecMapper<NormalizerParams, FacetedUnitSpec

const specWithReplacedEncoding = {
...spec,
name: spec.name && params.repeaterPrefix ? params.repeaterPrefix + spec.name : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

This will remove the name when there is no repeater prefix, which is definitely wrong (see changed example in this diff).

@ChristopherDavisUCI
Copy link
Contributor Author

Thanks @domoritz for proposing the fix in #8733! I will close this, but please let me know if I misunderstood anything.

@ChristopherDavisUCI ChristopherDavisUCI deleted the cd/prefix-in-layer-names branch August 14, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Altair Issue that is blocking Altair
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants