Skip to content

Attempt to fix #154 - #179

Closed
Chad Hietala (chadhietala) wants to merge 1 commit into
microsoft:masterfrom
chadhietala:strings-methods
Closed

Attempt to fix #154#179
Chad Hietala (chadhietala) wants to merge 1 commit into
microsoft:masterfrom
chadhietala:strings-methods

Conversation

@chadhietala

@chadhietala Chad Hietala (chadhietala) commented Jun 7, 2016

Copy link
Copy Markdown

I actually have no idea if this is correct or not but thought I would put it up to get some feedback.


_Edit by Daniel Rosenwasser (@DanielRosenwasser):_ Fixes #154.

@msftclas

Hi Chad Hietala (@chadhietala), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@DanielRosenwasser

Copy link
Copy Markdown
Member

Hey Chad Hietala (@chadhietala), as a heads up, mentioning the issue you're fixing in the PR body actually links to it, and the PR title itself should typically be more descriptive so readers don't have to navigate to the original issue for full context.

Anubha Mathur (@anubmat) can you take a look at this?

@msftclas

Chad Hietala (@chadhietala), Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@chadhietala

Chad Hietala (chadhietala) commented Jun 24, 2016

Copy link
Copy Markdown
Author

Not sure how important the CLA is now that I will be working at a MS subsidiary... 😄

Comment thread .vscode/settings.json Outdated
"editor.insertSpaces": true
} No newline at end of file
"editor.insertSpaces": true,
"files.trimTrailingWhitespace": false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why this change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I can remove this. I think I'm using this extension that adds a new line and tests care about whitespace.

@msftclas

Chad Hietala (@chadhietala), Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

method-declaration:
name: meta.method.declaration.ts
begin: '\b(?:(abstract)\s+)?\b(?:(public|private|protected)\s+)?\b(?:(async)\s+)?(?:(get|set)\s+)?(?:(new)|(?:\b(constructor)\b)|(?:([a-zA-Z_$][\.\w$]*)\s*(\??)))?\s*(?=\(|\<)'
begin: '\b(?:(abstract)\s+)?\b(?:(public|private|protected)\s+)?\b(?:(async)\s+)?(?:(get|set)\s+)?(?:(new)|(?:\b(constructor)\b)|(?:(\[*)(`|'|"*)(.*)(`|'|"*)(\]*)\s*(\??)))?\s*(?=\(|\<)'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This still doesn't look correct. For example, the .* would eat everything including the closing quote and bracket (the test case needs to be updated for it). Also, do you need to consider if the two quotes are matching? For example, ["test'] and ['test"] shouldn't be treated the same way as the ["test"]

@mhegazy

Copy link
Copy Markdown
Contributor

The original issue should be fixed now by #257

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.

5 participants