-
Notifications
You must be signed in to change notification settings - Fork 10
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
format: fix indentation for hotkeys {:: and }:: #12
Conversation
3 commits from fade2gray (Nov 13, 2020) https://github.com/fade2gray/vscode-autohotkey-plus-plus/tree/master grammar: createEditorListenr->createEditorListener
please do not use regex when not necessary added purity_greedy in codeUtil.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I'm not super familiar with formattingProvider.ts
yet, I'm comfortable merging to dev
once you make the requested changes. The "If you like" comments are bonus points 😄
@@ -55,7 +55,8 @@ export class FormatProvider implements vscode.DocumentFormattingEditProvider { | |||
} | |||
continue; | |||
} | |||
const purityText = CodeUtil.purity(originText.toLowerCase()); | |||
const purityText = CodeUtil.purity_greedy(originText.toLowerCase()); | |||
// console.log(purityText) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this commented out console log
if (purityText.match(/\b(return)\b/i) && tagDeep === deep) { | ||
tagDeep == 0; | ||
tagDeep == 0; //this does nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this line does nothing, then remove it. If we cannot verify whether it does anything, we should leave it in with a // TODO: does this do anything?
comment
public static purity_greedy(origin: string): string { | ||
if (!origin) return ''; | ||
// TODO: untest | ||
return origin | ||
.replace(/;.+/, '') | ||
.replace(/".*?"/g, '') | ||
.replace(/\{.*\}/g, '') | ||
.replace(/ +/g, ' ') | ||
.replace(/\bgui\b.*/gi, '') | ||
.replace(/\b(msgbox)\b.+?%/gi, '$1'); | ||
} | ||
|
||
public static purity(origin: string, greedy: boolean = false): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to make this a new function, just remove the unused greedy
parameter from purity
instead and call that. I see you also remove some unnecessary ?
matchers, feel free to remove those from the original purity
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that greedy parameter, and decided against it, because this is called EVERY line, and if statement is expensive,
what would be cool would be to replace purity
with purity_greedy
everywhere, but idk if it's a breaking change
I forgot to remove that unused parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be a breaking change. What is the difference between purity_greedy
and purity
? I see none. Just use purity
since we now have two functions doing the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it really isn't clear. it doesn't even show it.
line 12
.replace(/\{.*\}/g, '')
line 24
.replace(/\{.*?\}/g, '')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that. Removing the ?
from the regex does make it appear simpler, but they have the same functionality. Since the ?
follows a *
, it never does anything. The *
following the .
captures 0 or more instances of any char and the ?
captures 0 or 1 instance(s) of any char. With or without the ?
, the regex captures 0 or more instances of any char.
If I'm wrong, please provide a counterexample. For simplicity, consider be*?d
and note that it captures:
bd
bed
beed
Now consider be*d
. It captures the exact same things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend you go ahead and replace all *?
with *
for simplicity. I can manually test the functionalities that depend on purity
if you don't feel comfortable doing it. I still don't see any cause for having both purity_greedy
and purity
since they do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regex lesson: normally ? means 0 or 1 instance(s), but when ? is placed after *, it transforms the * into lazy, because * is greedy by Default
you can use https://regexr.com/ or https://regex101.com/ to help understand regex.
counterexample (use one of the 2 websites), the RegExp is code
and line below RegExp is the text string:
\{.*\}
fwefwefwef{very greedy } more cake}wefwefwefwef
\{.*?\}
fwefwefwef{nope, I've had enough } more cake}wefwefwefwef
I will have to find and understand the other use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Then of course don't change the regex! I will say, this PR is a bit stale. I am looking into fixing the last January bug now (#18), hopefully I will learn a bit about the formatter
deep -= temp; | ||
if (temp > 0) { | ||
notDeep = false; | ||
if (purityText.includes("}")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap strings in single quotes, not double quotes, throughout.
e.g. if (purityText.includes('}')) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: add eslint to devDependencies and in .vscode/settings.json
"editor.codeActionsOnSave": {
"source.fixAll.eslint": true
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can do that in another PR. ESLint is on the backlog. For now, would you mind changing them manually? If not, I suppose I can still merge the PR.
.replace(/\b(msgbox)\b.+?%/gi, '$1'); | ||
} | ||
|
||
public static purity(origin: string, greedy: boolean = false): string { | ||
if (!origin) return ''; | ||
// TODO: untest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like, I'd appreciate removing this comment, I don't know what it means.
} | ||
notDeep = false; | ||
} | ||
|
||
if (oneCommandCode && purityText.match(/{/) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like, you can remove this use of regex: ... && purityText.includes('{')) {
I can remove "unwanted" stuff. I don't want to remove my stuff EVERY TIME I want to merge here. the unwanted stuff can even be lines, or EVEN words, in file Is there already a way to do this ? |
RE removing unwanted stuff: there's always a way! Git is pretty good with ignoring entire files, as I'm sure you know. Just add something to the .gitignore. You can also look up "git update-index --assume-unchanged". What things are you talking about, specifically? I think we can figure out a way to manage this better when I know the specifics. Please open a new discussion so we can discuss this further, I don't want to clutter the PR thread with a general discussion |
Hi @FuPeiJiang, would you mind making these changes? I would love to finally merge this |
@FuPeiJiang any plans to update this PR? |
Closing as stale, @FuPeiJiang you're welcome to re-investigate this issue and open another PR. |
having {:: or }:: as hotkey breaks the indentation...
you can reproduce issue with \format_test_cases\{} match testing.ahk