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

PR to fix my previous issues. #1277

Merged
merged 8 commits into from
Dec 28, 2023

Conversation

hsyhhssyy
Copy link
Contributor

@hsyhhssyy hsyhhssyy commented Dec 6, 2023

The current version of the plugin still has several unresolved issues that I have encountered, detailed as follows:

  1. When openning server-side Classes using ObjectScript Explorer, pressing Ctrl+S and select a folder from the current workspace invariably prompts a question about overwriting. This happens because, upon file creation, the plugin automatically inserts a stub class code which often has a) an incorrect Package, and b) invariably triggers an overwrite prompt.
  2. The issue of the Class file's Package being altered during movement persists in the latest version of the plugin. Every scenario I reported in my previous case still occurs.
  3. When a file is a binary file but has a .cls extension, dragging it between folders in VSCode can corrupt its binary structure.

To thoroughly address these issues, I have submitted this PR. The PR introduces a new VSCode option designed to prevent any automated Code Formatter actions without explicit user permission. Any write operations to code files not executed through the user's ContextMenu will be disabled.

Currently, it only acts on onDidCreateFiles to prevent the automatic addition of PackageNames to code. If further similar behaviors are identified in the future, I plan to extend its control scope. For instance, if an ObjectScript Lint is introduced later, the option would block the automatic application of Lint when saving files with Ctrl+S. Users would need to explicitly apply Lint by right-clicking in the file and selecting FormatCurrentFile.

@hsyhhssyy
Copy link
Contributor Author

Related Issue: #1256

Copy link
Contributor

@isc-bsaviano isc-bsaviano left a comment

Choose a reason for hiding this comment

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

@hsyhhssyy Thank for the PR. I've added some suggestions below.

@gjsjohnmurray @isc-rsingh Can you take a look at this as well? I think the language of the new setting needs some work.

package.json Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@hsyhhssyy
Copy link
Contributor Author

I've accepted these suggestions and committed new code.

gjsjohnmurray
gjsjohnmurray previously approved these changes Dec 19, 2023
isc-rsingh
isc-rsingh previously approved these changes Dec 27, 2023
@isc-bsaviano
Copy link
Contributor

@hsyhhssyy Thanks for making the suggested changes! I'm getting ready to merge this PR, but one of our CI tests is failing due to linting issues. Can you run npm run lint-fix in your branch and push the changes that get made?

@hsyhhssyy hsyhhssyy dismissed stale reviews from isc-rsingh and gjsjohnmurray via a3737cf December 28, 2023 05:46
@hsyhhssyy
Copy link
Contributor Author

I have executed npm run lint-fix and submitted a new commit.

@isc-bsaviano isc-bsaviano merged commit 2872eb1 into intersystems-community:master Dec 28, 2023
4 of 7 checks passed
@isc-bsaviano
Copy link
Contributor

Thanks @hsyhhssyy, I just merged your PR!

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.

4 participants