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

Sanitize library names before running liblist command #1673

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

chrjorgensen
Copy link
Collaborator

Changes

This PR will fix the error described in issue #1672.
It will also fix errors in other library list commands:

  • show an error when changing current library, if the new current library name starts with #
  • don't allow non-existing libraries with name starting with # when changing the library list

Checklist

  • have tested my change
  • eslint is not complaining

@chrjorgensen chrjorgensen added the bug A confirmed issue when something isn't working as intended label Nov 20, 2023
@chrjorgensen chrjorgensen added this to the 2.5.0 release milestone Nov 20, 2023
@worksofliam worksofliam self-assigned this Nov 21, 2023
src/views/LibraryListView.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

I'd consider this a high impact change since it is used by a lot of our extension (and others!).

The tests are passing and things are working as normal.

Other than the unused import, the code and functionality works good.

Optionally, if you felt inclined, I'd love to see a new test case to show that it works with libraries that start with #.

@worksofliam worksofliam linked an issue Nov 21, 2023 that may be closed by this pull request
@chrjorgensen
Copy link
Collaborator Author

chrjorgensen commented Nov 21, 2023

@worksofliam I'm currently working on some tests - which revealed some defects with the handling of '$'!
We have IT auditing (💩) at work tomorrow, but I hope to have some fixes ready tomorrow evening or soon after...

EDIT: The '$' defect seems to be an old error - one can add '$LIBRARY' to the library list in the current version of C4i!

@chrjorgensen
Copy link
Collaborator Author

@worksofliam I fixed errors for sanitizing names with '$' and validating library list with sanitized names - and finally added some tests to verify library name sanitizing. Please have another go...

Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Looking good @chrjorgensen! Thanks for your work here.

@worksofliam worksofliam merged commit f584602 into codefori:master Nov 27, 2023
1 check passed
@chrjorgensen chrjorgensen deleted the fix/issue-1672 branch December 12, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A confirmed issue when something isn't working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QSH Compilation Command Fails When Library Name Contains '#'
2 participants