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

potential fix for issue#4134 #4136

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

the-dg04
Copy link

Potential fix for the problem

dos/bat/cmd Disk switch "D:" destroys highlight

Resolves #4134

Changes

in src/languages/dos.js line:151 replaced end:'goto:eof' to end:'\n'

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

@@ -148,7 +148,7 @@ export default function(hljs) {
{
className: 'function',
begin: LABEL.begin,
end: 'goto:eof',
end: '\n',
Copy link
Member

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.

Copy link
Author

@the-dg04 the-dg04 Oct 12, 2024

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.

Comment on lines 17 to 23
begin: '^\\s*[A-Za-z._?][A-Za-z0-9_$#@~.?]*(:|\\s+label)',
begin: '^:',
Copy link
Member

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?

Copy link
Member

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.

@the-dg04
Copy link
Author

I've added multiple possible cases for the use of : in a dos-script. Although I'm a bit muddled on what the relevance values should be for the new symbols. Mind helping me with that?

@joshgoebel
Copy link
Member

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]:\\?$',
Copy link
Member

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(
/^::.*$/, /$/,
Copy link
Member

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]+',
Copy link
Member

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*' }) ]
Copy link
Member

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*' }) ]
Copy link
Member

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?

@joshgoebel
Copy link
Member

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)

disk = [A-Z]:\\
path_component = [A-Za-z.....]
[disk]([path component]\\)*[path component]?

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?

@joshgoebel
Copy link
Member

Ping.

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.

dos/bat/cmd Disk switch "D:" destroys highlight
2 participants