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 spelling discrepancies #51

Merged
merged 3 commits into from Aug 12, 2021

Conversation

k-kuroguro
Copy link
Contributor

I found mixing 'writable' and 'writeable' in your code, fix 'writable' to 'writeable', because 'writeable' was there from the initial commit, and 'writable' seemed to have been added later.

@alefragnani
Copy link
Owner

Hi @k-kuroguro ,

This kind of typo (on settings) are the worst, because fixing it will make those settings stop to work 😞 .

I'm not sure how widely used are, so I don't know it worth a migration routine, but I'll see.

Thank you

@alefragnani alefragnani added this to the July 2021 milestone Jul 4, 2021
@alefragnani alefragnani added the bug label Jul 4, 2021
@k-kuroguro
Copy link
Contributor Author

k-kuroguro commented Jul 5, 2021

Hi @alefragnani, thanks for reviewing.

You're right, we must be cautious about updates.
So, I think you might want to leave the current configuration as deprecated like the following.

{
   "fileAccess.hideWhenWritable": {
      "type": "boolean",
      "default": false,
      "description": "Hide the Status Bar indicator when the file is Writable",
      "markdownDeprecationMessage": "**Deprecated**: Please use `#fileAccess.hideWhenWriteable#` instead.",
      "deprecationMessage": "Deprecated: Please use fileAccess.hideWhenWriteable instead."
   },
   "fileAccess.hideWhenWriteable": {
      "type": "boolean",
      "default": false,
      "description": "Hide the Status Bar indicator when the file is Writeable"
   }
}

@alefragnani
Copy link
Owner

Hi @k-kuroguro ,

I think deprecating is a good alternative. 👍

But, you could simplify the message, removing the Deprecated: prefix. Simply use "Use fileAccess.hideWhenWriteable instead.".

Thank you

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@k-kuroguro
Copy link
Contributor Author

k-kuroguro commented Jul 29, 2021

Hi @alefragnani , thanks for replying.

I have a little problem about deprecated settings.

  1. contributes.colors doesnt have deprecationMessage field.
  2. I want to check whether the new or old settings are being used for compatibility. As bellow
const config = typeof workspace.getConfiguration("fileAccess").get('hideWhenWriteable') === 'undefined'
   ? workspace.getConfiguration("fileAccess").get("hideWhenWritable", false)
   : workspace.getConfiguration("fileAccess").get("hideWhenWriteable", false);
but `get()` function does not returns undefined because it has default value as false in `package.json`.

I'll change as following

  1. Write "Deprecated: Use fileAccess.writeableForeground instead." on description field instead of deprecationMessage.
  2. Remove default value in package.json.

Both will remove the existing code. Is there a better way to do this?

@alefragnani
Copy link
Owner

Hi @k-kuroguro ,

Well, I didn’t notice contributes.colors doesn’t have deprecation. In this case, no need for special treatment.

Simply update the setting. No need to check for old values or anything like that. The old value will be ignored, the user will open settings.json to check and it will see it underlined. In the end, the user will fix the typo 😬 .

@k-kuroguro
Copy link
Contributor Author

@alefragnani, thanks for answering!

I add deprecated msg only contributes.configuration as you said.

@alefragnani alefragnani merged commit 56d04a9 into alefragnani:master Aug 12, 2021
@k-kuroguro k-kuroguro deleted the fix-spellingDiscrepancies branch August 12, 2021 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants