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 and test case for supporting members with dollar signs #2191

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

worksofliam
Copy link
Contributor

Changes

Fixes #2177 by not escaping at the right time.

  • A new test case with members with dollar signs in the name has been added
  • Renamed sanitise to noEscape to make more sense based on the existing code logic

How to test this PR

Examples:

  1. Run the test cases
  2. Try creating and editing a member with a dollar sign

Checklist

  • have tested my change
  • have created one or more test cases
  • updated relevant documentation
  • Remove any/all console.logs I added
  • have added myself to the contributors' list in CONTRIBUTING.md

Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam
Copy link
Contributor Author

worksofliam commented Jul 23, 2024

It seems like there might be some contention between where the encoding actually happens. I am passing true to qualifyPath so it doesn't escape:

https://github.com/codefori/vscode-ibmi/blob/fix/dollars_in_names/src/api/IBMiContent.ts#L152

As I am expecting it to be escaped in the runCommand function:

https://github.com/codefori/vscode-ibmi/blob/fix/dollars_in_names/src/api/CompileTools.ts#L681

@worksofliam worksofliam added this to the 2.12.0 milestone Jul 29, 2024
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Replacing sanitize with noEscape is great...makes a lot more sense!
See my remark about that line being removed though.

src/api/IBMiContent.ts Outdated Show resolved Hide resolved
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam
Copy link
Contributor Author

@sebjulliand Not sure why I did that. Re-requested your review.

Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Approved and good to go!

@worksofliam worksofliam merged commit ca2a54a into master Aug 5, 2024
1 check passed
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.

Opening files with $ sign in name
2 participants