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

Better Highlighting for Blocks #4520

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

riverdusty
Copy link

This enables proper variable highlighting when there are three pipes in a row such as in a block that accepts an argument, thing do: [:e | | var1 var2 | ... ].

@riverdusty
Copy link
Author

freekin lint and stuff.

@riverdusty riverdusty closed this Jan 31, 2017
@riverdusty riverdusty reopened this Jan 31, 2017
@adrianheine
Copy link
Contributor

Hi, thanks for your pull request. Your changes are quite difficult to review currently since they include a lot of unrelated white-space changes. Could you try to update your fork's branch so that it doesn't include these changes? See Why and how to correctly amend GitHub pull requests for how to do this.

@riverdusty
Copy link
Author

That should be easier. Not terrible for my first pull request using github, but I've learned and will try to remember. Thanks!

@riverdusty riverdusty closed this Feb 9, 2017
@riverdusty riverdusty reopened this Feb 9, 2017
@@ -91,6 +92,7 @@ CodeMirror.defineMode('smalltalk', function(config) {
} else if (/[\w_]/.test(aChar)) {
stream.eatWhile(/[\w\d_]/);
token.name = state.expectVariable ? (keywords.test(stream.current()) ? 'keyword' : 'variable') : null;
token.name = state.expectVariable ? (variables.test(stream.current()) ? 'variable-2' : 'variable') : null;
Copy link
Member

Choose a reason for hiding this comment

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

Why are you assigning to the same property twice here? This effectively disables the previous statement, which doesn't look like it is right.

Copy link
Author

Choose a reason for hiding this comment

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

Could probably be better represented like,
} else if (/[\w_]/.test(aChar)) { stream.eatWhile(/[\w\d_]/); if(keywords.test(stream.current()){ token.name = 'keyword' } else if(variables.test(stream.current){ token.name = 'variable-2' } else { token.name = 'variable' }

@@ -15,6 +15,7 @@ CodeMirror.defineMode('smalltalk', function(config) {

var specialChars = /[+\-\/\\*~<>=@%|&?!.,:;^]/;
var keywords = /true|false|nil|self|super|thisContext/;
var variables = /Smalltalk/;
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't use a mode-global variable to keep state -- modes can be restarted and resumed at different points at any time, and thus state should go into the state object.

Copy link
Author

Choose a reason for hiding this comment

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

I feel like I'm in a lecture! heh. I will make a note of this and a) fix and b) not do it again!

@@ -120,6 +122,17 @@ CodeMirror.defineMode('smalltalk', function(config) {

if (aChar === '|') {
token.context = context.parent;
var str = stream.string;
Copy link
Member

Choose a reason for hiding this comment

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

Could whatever you're trying to do here be expressed by matching a regexp on the stream at this point? Munging the whole line without regard for where in the line we currently are doesn't seem like a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a JS developer by trade, but I am learning. I'll give the RegEx a shot. Basically I'm trying to find whether or not there are additional variables in a block, so [:var1 | |var2 var3| ...]

arr.length == 3 ? str = arr[1] : str = arr[2];
if(!str){ str = '' };
var arr2 = str.split(' ');
for(var i=0;i<arr2.length;i++){
Copy link
Member

Choose a reason for hiding this comment

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

(Also please put spaces around operators.)

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.

3 participants