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: Expand allowed characters in keys #4044 #4045

Closed
wants to merge 7 commits into from

Conversation

uniqueeest
Copy link

Changes

This modification extends the definition of the key to include spaces, colon, slash, hyphen, and parentheses. It also changes the key to include more character types while maintaining the pattern that ends in colon.

Checklist

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

src/languages/yaml.js Outdated Show resolved Hide resolved
Comment on lines 24 to 28
{ begin: /\w[\w :()\./\-*@]*:(?=[ \t]|$)/ },
{ // double quoted keys - with brackets
begin: /"\w[\w :()\./-]*":(?=[ \t]|$)/ },
begin: /"\w[\w :()\./\-*@]*":(?=[ \t]|$)/ },
{ // single quoted keys - with brackets
begin: /'\w[\w :()\./-]*':(?=[ \t]|$)/ },
begin: /'\w[\w :()\./\-*@]*':(?=[ \t]|$)/ },
Copy link
Member

@joshgoebel joshgoebel May 1, 2024

Choose a reason for hiding this comment

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

Awesome!

Can we please keep the - at the very end of the character group so we don't need the extra escape?

Also, could we get a test case added to the markup tests to show that this works as intended?

Copy link
Author

Choose a reason for hiding this comment

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

@joshgoebel I moved the location of - and created a test file. When running the test, it looks like the picture below, is that right?
image

Copy link
Author

Choose a reason for hiding this comment

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

hello! @joshgoebel. Can you please review this part?

@joshgoebel
Copy link
Member

How are you generating the expect file? All literal quotes need to be HTML escaped.

@uniqueeest
Copy link
Author

@joshgoebel I wrote it.

expect-test

<span class="hljs-attr">"*":</span>

test

"*":

But it comes out like the picture I posted.

@joshgoebel
Copy link
Member

Every " should be &quot; as the failing test is telling you. The output from Highlight.js must be HTML encoded.

@uniqueeest
Copy link
Author

@joshgoebel Test file modification completed. It also passed without any problems.


"octo-repo":
Copy link
Member

Choose a reason for hiding this comment

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

Why did we lose all the keys?

Copy link
Author

Choose a reason for hiding this comment

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

@joshgoebel It wasn't a key value that needed to be tested, but a title for that test😂. I was mistaken at first and added that part to the test.

@@ -20,12 +20,12 @@ export default function(hljs) {
const KEY = {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a look-ahead for : at the end perhaps to make sure these are keys?

Copy link
Author

Choose a reason for hiding this comment

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

@joshgoebel Should I add it if I think I need it? How should I do it

@joshgoebel
Copy link
Member

#4105 has solved this slightly more flexibly. Sorry.

@joshgoebel joshgoebel closed this Oct 21, 2024
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.

2 participants