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

Correct property names and improve scope-names #54

Merged
merged 5 commits into from
Sep 15, 2016
Merged

Correct property names and improve scope-names #54

merged 5 commits into from
Sep 15, 2016

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Sep 10, 2016

Fixes #48 and fixes #53.

@Ingramz
Copy link
Collaborator

Ingramz commented Sep 10, 2016

Thanks, I'll check it out.

'0':
'name': 'support.punctuation.blade'
'1':
'name': 'source.php'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leave these kind captures in, this is a hack required for text editors for properly detect inner scope when the contents of the block is 0 width, like {{|}} where | is cursor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a way to trigger PHP-specific snippets and commands, I assume?

Why not move the source.php scope to the containing block's name property, then...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've restored the capturing groups, but you should perhaps consider adding a comment to elaborate on why they're necessary. Future contributors may also misinterpret those groups as a mistake.

'name': 'support.punctuation.blade'
'1':
'name': 'source.php'
'name': 'punctuation.section.begin.embedded.blade'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move .begin and .end after .embedded like you did with comment.

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 13, 2016

@Ingramz There's an emergency release of Linguist due to be cut pretty soon, and it'd be nice to get these highlighting issues fixed on the site instead of wait a few more weeks. :)

@Ingramz
Copy link
Collaborator

Ingramz commented Sep 13, 2016

@Alhadis alright. It'll have to wait till I am done with my work commitments for the day, though.

@Ingramz
Copy link
Collaborator

Ingramz commented Sep 15, 2016

@Alhadis I'm sorry it didn't make the cut (pun intended). But here's my offer regarding scope names:
https://docs.google.com/spreadsheets/d/1bTwlO-cSer1rI4lY-IptddjYUQq4dkJiudM1gTB1BgU/edit?usp=sharing

Bold marks differences. Let me know what you think. I think it's a slightly better compromise between properly defined names and not looking like crap, but definitely not perfect. I will probably start splitting built-in directives into smaller groups by their function, eg control keywords and other keywords, but let that be for now.

Is there any way to test this configuration against linguist ahead of time?

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 15, 2016

Is there any way to test this configuration against linguist ahead of time?

Well, there's a webapp called Lightshow which will give a general impression, but its highlighting palette is outdated and the app itself is buggy. Should be enough though.

@Ingramz
Copy link
Collaborator

Ingramz commented Sep 15, 2016

At least with lightshow the echo mustaches look pretty now 😸

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 15, 2016

Well, at the end of the day, that's all that matters, I guess. :)

Pretty moustaches an' shit.

'contentName': 'source.php'
'name': 'meta.embedded.blade'
'name': 'meta.embedded.keyword.blade'
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be meta.embedded.block.blade

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remedied.

@Ingramz
Copy link
Collaborator

Ingramz commented Sep 15, 2016

Sweet. Expect a release very soon™.

@Ingramz Ingramz merged commit c2fc80e into jawee:master Sep 15, 2016
@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 15, 2016

Ace, now people will shaddap at Linguist. 👍

@Alhadis Alhadis deleted the scope-names branch September 15, 2016 13:56
@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 15, 2016

Aitäh!

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.

GitHub syntax Scope names are inconsistent
2 participants