Add support for "SINUMERIK 840D language support" package. #379
Conversation
| { | ||
| "name": "s840d_gcode", | ||
| "open": "(?i)\\b(IF(?!.*GOTO)|FOR|WHILE|REPEAT(?!.*GOTO))\\b", | ||
| "close": "(?i)\\b(END(?:IF|FOR|WHILE)|UNTIL)\\b", |
There was a problem hiding this comment.
Is UNTIL meant to be REPEAT? If so, you might want to implement a bh_module to ensure this pairing. You can check out existing bh_modules. Maybe this one would help: https://github.com/facelessuser/BracketHighlighter/blob/master/bh_modules/luakeywords.py.
| // SINUMERIK 840D SL G-Code | ||
| { | ||
| "name": "s840d_gcode", | ||
| "open": "(?i)\\b(IF(?!.*GOTO)|FOR|WHILE|REPEAT(?!.*GOTO))\\b", |
There was a problem hiding this comment.
Don't use (?i), it should already be case insensitive. If you need to enforce case, do it in a bh_module. These flags will affect all currently active rules.
…MERIK 840D language support"
… language support" package
|
Added bh_modules for both syntaxes and removed the (?i) tags. |
…MERIK 840D language support" package
|
|
||
| match = False | ||
| # classes | ||
| if o in ["//a", "//b", "//g", "//m", "//s"] and c == "//end": |
There was a problem hiding this comment.
Instead of creating a new list instance every time the compare method is called, how about creating a static tuple up front?
S840D_HMI_CLASSES = ("//a", "//b", "//g", "//m", "//s")
...
if o in S840D_HMI_CLASSES and c == "//end":
See the following for reference: https://github.com/facelessuser/BracketHighlighter/blob/master/bh_modules/rubykeywords.py.
We don't really need a dynamic list and tuples are faster to assign to. And this way we only bother allocating the object when the file is loaded, after that all calls to the compare method just references the object.
Everything else looks fine.
|
This is actually a good idea. I unfortunatelly took the bashsupport.py as an example, which creates a list, too. So I would suggest to change it to use static tubles, too. |
|
I agree. I am trying to be more thorough, but it doesn't surprise me that bad examples exist from previous pulls. |
| @@ -8,6 +8,9 @@ | |||
| lowercase = import_module("bh_modules.lowercase") | |||
|
|
|||
|
|
|||
| BASH_KEYWORDS = ("select", "for", "while", "until") | |||
There was a problem hiding this comment.
Only one blank line is needed before this. You will only see two before functions and classes in the global scope. It's a Python PEP8 thing.
| """ | ||
|
|
||
|
|
||
| S840D_HMI_CLASSES = ("//a", "//b", "//g", "//m", "//s") |
There was a problem hiding this comment.
Same as mentioned previously. Only one blank line before this.
|
You are very thorough now, even more than me. ;-) Neither SublimeLinter nor your travis-ci displayed an linting error for this blank line. But I agree, with you and removed it to make code as clean as possible. |
|
Yeah, I was surprised the linter didn't pick it up. I would have easily missed it if it wasn't the only change in that last revision :). Thanks for the pull. This should make it in the next release. |
Could you please add bracket rules for my CNC SINUMERIK 840D package, please?
see here