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

Remove no-op pops. #3530

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Remove no-op pops. #3530

merged 1 commit into from
Oct 8, 2024

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Oct 4, 2024 •
edited by github-actions bot
Loading

BugDX-3087 Buildexpression unmarshalling has potential pathing issues

We still want to keep context, but popping is not necessary since mutations are done locally (path = append(path, ...)), and do not affect the values passed by callers.

@mitchell-as mitchell-as requested a review from MDrakos October 4, 2024 20:04
@mitchell-as mitchell-as marked this pull request as ready for review October 4, 2024 20:04
MDrakos
MDrakos approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

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

Do we even need the path value at all? The pop calls were doing nothing here but that means that the path value is likely incorrect in many places.

@mitchell-as
Copy link
Contributor Author

We do need path, and the values were correct in the locations they were used.

@mitchell-as mitchell-as merged commit cdaf2ae into version/0-47-0-RC1 Oct 8, 2024
13 of 18 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-3087 branch October 8, 2024 16:44
as-builds added a commit that referenced this pull request Oct 8, 2024
@as-builds
Merge branch version/0-47-0-RC1 to adopt changes from PR #3530
3479068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MDrakos MDrakos MDrakos approved these changes

Assignees
No one assigned
Labels
Test: buildscripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mitchell-as @MDrakos