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

Mass renaming proposal #1404

Open
33 tasks
carsakiller opened this issue Jul 28, 2022 · 10 comments
Open
33 tasks

Mass renaming proposal #1404

carsakiller opened this issue Jul 28, 2022 · 10 comments
Labels
suggestion More general suggestions that may be more opinionated
Milestone

Comments

@carsakiller
Copy link
Collaborator

carsakiller commented Jul 28, 2022

There are a number of settings, diagnostics, and more that have unclear names that I think can be improved. This issue serves to propose new names for these items so that they can all be tracked in one place and implemented at once to minimize the number of breaking releases that would need to be released.

Settings

  • completion.displayContext -> completion.peekDefinition
    • There is already a Peek Definition command in VS Code that has a similar function and I think naming it the same will help people understand its function without confusing the two as they are triggered entirely differently.
  • diagnostics.disable -> REMOVE
  • diagnostics.groupFileStatus -> REMOVE
  • diagnostics.groupSeverity -> REMOVE
  • diagnostics.ignoreFiles -> diagnostics.diagnoseIgnored
  • diagnostics.libraryFiles -> diagnostics.diagnoseLibraries
  • diagnostics.neededFileStatus -> diagnostics.config (Rework functionality)
  • diagnostics.severity (Rework functionality)
  • diagnostics.workspaceDelay -> diagnostics.delay
  • hint.setType -> hint.assignType
  • hover.enumsLimit -> hover.typesLimit
  • hover.previewFields -> hover.peekFields
  • runtime.special -> runtime.aliases
  • workspace.checkThirdParty -> workspace.checkEnvConfigs
  • workspace.ignoreDir -> workspace.ignore
  • workspace.library -> workspace.libraries
  • workspace.maxPreload -> workspace.maxFiles
  • workspace.preloadFileSize -> workspace.maxFileSize
  • workspace.userThirdParty -> workspace.envConfig

Diagnostics

I imagine there is a good reason abiguity-1 is named how it is 😄 I see they all have at least 1 hyphen so I guess that is a limitation with how they are parsed?

  • unknown-diag-code -> unknown-diagnostic
  • cast-local-type -> local-cast-mismatch

Syntax Errors

Luckily these can probably be changed with very little impact on the user and most people likely would not have any settings related to these.

  • err-assign-as-eq -> assign-with-equals
  • err-c-long-comment -> multiline-comment-prefix
  • err-comment-prefix -> comment-prefix
  • err-do-as-then -> then-as-do
  • err-eq-as-assign -> equality-with-assign
  • err-esc -> unknown-escape
  • err-nonstandard-symbol -> non-standard-symbol
  • err-then-as-do -> do-as-then
  • exp-in-action -> unexpected-expression
  • keyword -> reserved
  • miss-esc-x -> miss-hex-value
  • miss-exp -> miss-expression

newfield-call and newline-call are not easy to understand but I can't think of anything that does a better job 🤔

I don't even know what these do: unexpect-efunc-name, unexpect-lfunc-name


Please let me know what you think and feel free to suggest other names.

@sumneko
Copy link
Collaborator

sumneko commented Jul 29, 2022

unexpect-efunc-name:

local x = function x() end

unexpect-lfunc-name

local function x:t() end

@flrgh
Copy link
Contributor

flrgh commented Aug 1, 2022

👋

workspace.userThirdParty -> workspace.envEmulations

This one seems like a step in a very specific direction, and I'm not sure I agree with it.

Most of the existing third-party annotation libraries are very framework-y in nature, so referring to them as environments makes perfect sense. However, we also have at least one that is purely a standalone library though (lfs).

In my ideal scenario there would be support for both frameworks and many more libraries, which can be detected at runtime when the server sees require "{{name_of_library}}" in a file (I am personally working on type annotations for Penlight and luasocket). Calling the setting envEmulations makes the framework use case a little more intuitive, but it makes the library use case much less intuitive.

Maybe this specific feature needs its own discussion though. It's possible that supporting standalone libraries was never a goal for this feature anyhow.

@flrgh
Copy link
Contributor

flrgh commented Aug 1, 2022

Thanks @carsakiller for writing this up and starting the discussion

  • 👍 completion.displayContext -> completion.peekDefinition
  • 👍 diagnostics.*
    • seems like the discussion in the other thread is getting close to some consensus. I'm not a power user of this feature though so my opinion shouldn't count for much
  • 👍 hint.setType -> hint.assignType
  • 👍 hover.enumsLimit -> hover.typesLimit
  • 👍 / 👎 hover.previewFields -> hover.peekFields
    • seems like a fairly lateral change IMO, but no objection
  • 👍 / 👎 runtime.special -> runtime.specialVars
    • agree that it could be improved, just not sure about the name. How about runtime.globalAliases or something that communicates a little bit more about the effect that the setting has?
  • 👎 workspace.checkThirdParty -> workspace.checkEnvEmulations
    • see other comment
  • 👍 workspace.ignoreDir -> workspace.ignore
  • 👍 workspace.library -> workspace.libraries
  • 👍 workspace.maxPreload -> workspace.maxFiles
  • 👍 workspace.preloadFileSize -> workspace.maxFileSize
  • 👎 workspace.userThirdParty -> workspace.envEmulations
    • see other comment

Diagnostics & Syntax Errors

No opinions about these other than keeping them consistent with one another.

I also think it would be well worth the effort to add some code to help users transition to the new names by detecting the old ones and offering to migrate and save the settings for them. Users spend a lot of time tuning and tweaking their settings, and nothing is more frustrating than when a backwards-incompatible change invalidates hours of hard work (this is the sort of thing that has made me give up on using several otherwise-great editor plugins in the past).

@carsakiller
Copy link
Collaborator Author

Thanks for the response, it is great bouncing ideas around 🙂

Third Party Directories

My impression was always that third party directories were meant as a more in-depth solution that would not just offer definitions, but also be able to emulate a target environment as closely as possible. Whether you actually modify the environment through more than just definition files or not, I think that name is most fitting for the function of that feature. Using the third party directories to just automatically load definition files is still modifying the simulated environment, after all. I can see what you mean though, as if you are just trying to automatically apply definition files, environment emulation sounds a little too advanced/off-topic. Perhaps environment configuration would be better as both just definition files and full frameworks configure the environment.

hover.previewFields -> hover.peekFields

This suggestion is definitely one of the more insignificant ones as it is really just replacing a word with a synonym. My intention is, seeing as VS Code uses peek in a few areas already that have a similar function, I was hoping it would be more obvious how it will look. It is absolutely not a necessary change, I just figured that if we are already renaming so many things, it would be best to do it now even if it only offered marginal improvements at best.

runtime.special -> runtime.specialVars

I have personally never used, or had a need, to use this setting. Regardless, I do like your suggestion. I think we can do one better though and just do runtime.aliases.

Syntax Errors

I noticed that a lot of the syntax errors start with err-, but not all. I personally don't see much of a purpose of prefixing syntax errors with something that makes it more obvious that you are receiving a syntax error instead of a diagnostic warning, seeing as their purpose is the same. So maybe we remove all the err- prefixes?

@carsakiller
Copy link
Collaborator Author

I have edited this issue to add proposals for removing all (at least the ones I could find) err- prefixes for syntax errors.

I also changed environmentEmulation to environmentConfig - how does that sound @flrgh?

@FlashHit
Copy link

please ping me if anything gets changed, so I can adjust my workspace settings in time.

@carsakiller
Copy link
Collaborator Author

If there is going to be this many things changing, I think it will be something that everyone gets notified of in advance.

@sumneko sumneko added this to the 3.7.0 milestone Nov 7, 2022
@carsakiller
Copy link
Collaborator Author

@sumneko, unless the intention is to perform the rename in a backwards-compatible way (which would be nice, just sounds difficult), semver recommends that the changes result in a major version bump (4.0.0).

@carsakiller carsakiller added the suggestion More general suggestions that may be more opinionated label Nov 12, 2022
@sumneko
Copy link
Collaborator

sumneko commented Nov 12, 2022

It is not difficult to keep backwards-compatible

@hinell
Copy link

hinell commented Dec 7, 2023

I find increasingly confusing the following two options for LLS config:

The runtime.path is easily confused for lua or neovim runtime libraries, not current project's one "runtime" lua files whatever that means. I propose to soft-deprecate runtime.path in favor of workspace.input or workspace.files.

And the workspace.library in favor of workspace.dependencies or workspace.libraries like it was suggested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion More general suggestions that may be more opinionated
Projects
None yet
Development

No branches or pull requests

5 participants