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

Refactor ProgramResolver #513

Merged
merged 7 commits into from
Feb 26, 2024
Merged

Refactor ProgramResolver #513

merged 7 commits into from
Feb 26, 2024

Conversation

adambabik
Copy link
Collaborator

@adambabik adambabik commented Feb 25, 2024

Follow up for #512.

Copy link
Collaborator Author

@adambabik adambabik left a comment

Choose a reason for hiding this comment

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

@sourishkrout watch out for field names changed in runner.proto. There is no new enum values though or deleted/new fields. Status enum is not backward compatible, as STATUS_RESOLVED is the last one.

ModifiedProgram: true,
Variables: []ProgramResolverVarResult{
{
Status: ProgramResolverStatusUnresolved,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sourishkrout There are a few changes like this one in these tests, which I hope led to more consistent behaviour.

All variables are unresolved here and:

  • If a variable is empty (export A, export A=, export A="") it is marked as ProgramResolverStatusUnresolved.
  • If there is any unquoted value, it is marked as ProgramResolverStatusUnresolvedWithMessage.
  • If there is any quoted value, it is marked as ProgramResolverStatusUnresolvedWithPlaceholder.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair. In any case, the extension will likely choose to prompt for ProgramResolverStatusUnresolved with a generic message. 👍

require.NoError(t, err)
assert.EqualValues(t, tc.modifiedProgram, buf.String())
// In skip mode, all variables will be resolved.
if tc.result != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sourishkrout I hope I figured this out correctly: when ProgramResolverModeSkipAll, then all variables are marked as resolved and their resolved value is equal to the original value.

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

@adambabik adambabik marked this pull request as ready for review February 26, 2024 21:29
Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

👍 Thank you!

I will change the integration code on the extension-side. Do some acceptance testing and if successful merge this branch.

ModifiedProgram: true,
Variables: []ProgramResolverVarResult{
{
Status: ProgramResolverStatusUnresolved,
Copy link
Member

Choose a reason for hiding this comment

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

That's fair. In any case, the extension will likely choose to prompt for ProgramResolverStatusUnresolved with a generic message. 👍

require.NoError(t, err)
assert.EqualValues(t, tc.modifiedProgram, buf.String())
// In skip mode, all variables will be resolved.
if tc.result != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Yes!

Copy link

sonarcloud bot commented Feb 26, 2024

@sourishkrout sourishkrout merged commit b771cac into main Feb 26, 2024
6 checks passed
@sourishkrout sourishkrout deleted the adamb/refactor-resolve-program branch February 26, 2024 22:55
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