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

fix(basic) - closing quotation marks not required for a PRINT statement #4137

Merged
merged 9 commits into from
Oct 22, 2024

Conversation

somya-05
Copy link
Contributor

Fixed the closing quotations marks considered for string highlighting in basic.

Resolves #3996

Changes

Before
image

After
image

Checklist

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

hljs.QUOTE_STRING_MODE,
{
// Match strings that start with " and end with " or a line break
className: 'string',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
className: 'string',
scope: 'string',

{
// Match strings that start with " and end with " or a line break
className: 'string',
begin: '"',
Copy link
Member

Choose a reason for hiding this comment

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

Please use regex literals for all the regex, rather than strings.

@somya-05
Copy link
Contributor Author

@joshgoebel the changes for regex literals are made, please review them once.

scope: 'string',
begin: /"/,
end: /"|$/,
illegal: /\n/,
Copy link
Member

@joshgoebel joshgoebel Oct 20, 2024

Choose a reason for hiding this comment

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

I'm not sure this illegal has any effect at all since the $ should match first (or does it match after)... are you seeing different behavior with/without illegal?

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 added the illegal keyword to ensure the string doesn't span multiple lines. But I can't see a different behavior without it. This can be removed to optimize the code.

Copy link
Member

Choose a reason for hiding this comment

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

It can't span multiple lines because the $ will match the end of the first line.

@joshgoebel joshgoebel added the almost-there Getting close ot merging label Oct 20, 2024
Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +61 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB +2 B
es/highlight.min.js 8.18 KB 8.18 KB +2 B
es/languages/basic.min.js 1 KB 1.03 KB +28 B
highlight.min.js 8.22 KB 8.22 KB +2 B
languages/basic.min.js 1 KB 1.03 KB +27 B

@joshgoebel joshgoebel merged commit 4e485f2 into highlightjs:main Oct 22, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost-there Getting close ot merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(basic) Closing quotation marks are not required for a PRINT statement (gwbasic)
2 participants