-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
potential fix for issue#4134 #4136
base: main
Are you sure you want to change the base?
Conversation
src/languages/dos.js
Outdated
@@ -148,7 +148,7 @@ export default function(hljs) { | |||
{ | |||
className: 'function', | |||
begin: LABEL.begin, | |||
end: 'goto:eof', | |||
end: '\n', |
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.
Ok, that was a weird end condition, but now D:
is incorrectly detected as a label, is it not? From what I'm reading labels always start with :
not end with it... so could we just update our label matcher to something like?
[start of line]:[label_name]
I think that would wrap this up nicely.
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.
That makes sense, ive made it so that expressions like [start of line]:[label name]
are matched as labels and expressions like [start of line]::[something]
are not matched as they denote comments.
src/languages/dos.js
Outdated
begin: '^\\s*[A-Za-z._?][A-Za-z0-9_$#@~.?]*(:|\\s+label)', | ||
begin: '^:', |
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.
Wouldn't this be something more like:
/^:[A-Za-z._?][A-Za-z0-9_$#@~.?]*/
Or whatever value characters for a label are?
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'll have to take a closer look to see how this is currently working. This grammar pattern is pretty old... I think we could have a much simpler rule here, or perhaps a multi-match if we truly want the :
to be different than the name of the label.
I've added multiple possible cases for the use of |
We can just forget about relevance, we'll be dropping it from v12 anyways. It would be nice to add some markup examples here so I can see which cases you are trying to cover explicitly. Once I have some solid test cases I'll probably clean this up a bit further before merging. |
|
||
const DISK_CHANGE = { | ||
className: 'symbol', | ||
begin: '^[A-Za-z]:\\?$', |
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.
This should be a literal regex, not a string.
|
||
// for matching comments starting with :: | ||
const COMMENT_2 = hljs.COMMENT( | ||
/^::.*$/, /$/, |
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.
The begin should start and the end should end, so I think you want:
start: /^::/
end : /$/
The end shouldn't be included in the begin.
|
||
const OUTPUT_REDIRECT = { | ||
className: 'symbol', | ||
begin: '[1-2]?[>]>{1}\s*[^&\s]+', |
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.
Please add a markup test or two for this so I can understand what exactly you're trying to match. This looks a bit overly complex to me, but maybe I don't understand the goal.
className: 'function', | ||
begin: DISK_CHANGE.begin, | ||
end: /\n/, | ||
contains: [ hljs.inherit(hljs.TITLE_MODE, { begin: '([_a-zA-Z]\\w*\\.)*([_a-zA-Z]\\w*:)?[_a-zA-Z]\\w*' }) ] |
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.
Please explain what this is attempting to do. Are you trying to highlight all path names?
className: 'function', | ||
begin: OUTPUT_REDIRECT.begin, | ||
end: /\n/, | ||
contains: [ hljs.inherit(hljs.TITLE_MODE, { begin: '([_a-zA-Z]\\w*\\.)*([_a-zA-Z]\\w*:)?[_a-zA-Z]\\w*' }) ] |
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.
Same question, is this another attempt to highlight pathname?
The DOS grammar is simple enough we could try adding path highlighting, but we'd do it by recognizing the FULL path... ie something like: (pseudo code)
So first the disk, followed by any number of \ terminated path segments possibly following be a single non-slash terminated segment... and maybe one sep rule to match non-slash terminated disks... Does that make sense? |
Ping. |
Potential fix for the problem
dos/bat/cmd Disk switch "D:" destroys highlight
Resolves #4134
Changes
in
src/languages/dos.js line:151
replacedend:'goto:eof'
toend:'\n'
Checklist
CHANGES.md