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

change input type for concat #7054

Conversation

sunjh
Copy link

@sunjh sunjh commented Sep 24, 2024

Why the changes in this PR are needed?

This is to improve the concat function mentioned here [https://github.com//issues/6911](ISSUE 6911)

What are the changes in this PR?

Changed concat to accept both string & number

Notes to assist PR review:

#6911

Further comments:

Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit b729b83
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66f251fe54b22f000847b043
😎 Deploy Preview https://deploy-preview-7054--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit e3f58ad
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66f252277f37dd00087b6b18
😎 Deploy Preview https://deploy-preview-7054--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sunjh
Copy link
Author

sunjh commented Sep 24, 2024

Hi @anderseknert looks like it passed all the checks, please help review it, thanks!

@srenatus
Copy link
Contributor

Code looks good. An extra test in the yaml test cases would be a nice touch. I'm also afraid this won't work as-is with wasm 🤔

@sunjh
Copy link
Author

sunjh commented Sep 27, 2024

Code looks good. An extra test in the yaml test cases would be a nice touch. I'm also afraid this won't work as-is with wasm 🤔

Thanks @srenatus , I will try to add an extra test case for this.
Since I'm not using wasm, not familiar with it, would you mind share some docs about how to verify that in wasm? Thanks!

@srenatus
Copy link
Contributor

So as soon as we have a yaml tests case, the CI suite will run that yaml test case with standard eval (topdown) and wasm; and both tests need to succeed for the PR to go green.

Concretely, we'll need the same change you've done here for topdown in the wasm "base lib", which is a few C functions here:

@sunjh
Copy link
Author

sunjh commented Oct 8, 2024

So as soon as we have a yaml tests case, the CI suite will run that yaml test case with standard eval (topdown) and wasm; and both tests need to succeed for the PR to go green.

Concretely, we'll need the same change you've done here for topdown in the wasm "base lib", which is a few C functions here:

Thanks for the comments. Working on it.

@ashutosh-narkar
Copy link
Member

@sunjh thanks for working on this. Just checking in to see if you had a chance to address Stephan's comments. We can then get this in the next release.

@ashutosh-narkar
Copy link
Member

@sunjh closing this PR for now. Feel free to re-open when you get a chance to address the PR comments and we can get this in.

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.

3 participants