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.

Sign up for GitHub

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

Jump to bottom

thema: Fix Translate() to same version #144

Merged
merged 4 commits into from
May 11, 2023
Merged

thema: Fix Translate() to same version #144

merged 4 commits into from
May 11, 2023

Conversation

joanlopez
Copy link
Contributor

(Descendant of #141)

It fixes the Translate() operation when from and to versions are the same, because the current implementation produces an error in such case:

_objold.steps: conflicting values [...#TranslatedInstance] and {} (mismatched types list and struct):
    ./testdata/internal/schmoo.cue:55:10
    ./translate.cue:37:10
    ./translate.cue:45:10
    ./translate.cue:128:16

@joanlopez joanlopez requested a review from sdboyer May 11, 2023 00:01
@joanlopez joanlopez self-assigned this May 11, 2023
@joanlopez joanlopez added the bug Something isn't working label May 11, 2023
joanlopez
joanlopez commented May 11, 2023
View reviewed changes
testdata/lineage/basic-multiversion.txtar
Comment on lines +194 to +196
{"init":"some string"}
{"init":"some string","withDefault":"foo"}
[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it look more clear now?
It keeps the "one-liner idea" but writing all of them:

  • From (example)
  • To (translation result)
  • Lacunas

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is :chef-kiss:

joanlopez
joanlopez commented May 11, 2023
View reviewed changes
Copy link
Contributor Author

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Hey @sdboyer, sorry for the noise! I expected #141 target branch to be updated to main as soon as #139 got merged, but that haven't been the case.

Now most (or all) of the discussions from #141 should be solved. Thanks!

sdboyer
sdboyer approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

looks great. In we go!

@sdboyer sdboyer merged commit 9a5baae into main May 11, 2023
2 checks passed
@joanlopez joanlopez mentioned this pull request May 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdboyer sdboyer sdboyer approved these changes

Assignees

@joanlopez joanlopez

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@joanlopez @sdboyer