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

enh(typescript): highlight obvious user defined type names (interface, etc) #3903

Merged
merged 11 commits into from
Nov 16, 2023

Conversation

MohamedAli00949
Copy link
Contributor

I have fixed highlighting params at JS and these params types such as (interfaces, types, etc) at TS.

The related issue is #3269

Changes

My changes are concat the params regex contains with class reference regex and attribute regex to highlight props keys and types.
While I working on this issue, I noted that the params didn't highlight if it separated with space before and solved that also.

Checklist

  • Fix params highlighting at js functions.
  • Fix params types (interfaces, types, etc).
  • Added markup tests to test the changes.

@github-actions
Copy link

Build Size Report

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

7 files changed

Total change +275 B

View Changes
file base pr diff
es/core.min.js 8.14 KB 8.13 KB -1 B
es/highlight.min.js 8.14 KB 8.13 KB -1 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.21 KB +129 B
highlight.min.js 8.17 KB 8.17 KB -1 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.22 KB +130 B

@MohamedAli00949
Copy link
Contributor Author

MohamedAli00949 commented Oct 27, 2023

Hi @joshgoebel, this is my suggested solution and I hope to review it.

@MohamedAli00949
Copy link
Contributor Author

@joshgoebel, is there any opportunity to review these changes?
Thanks in advance

params.contains = params.contains.concat(PROPERTY_TYPE)
}

const returnedFuncParams = contains.find(cc => cc.scope === 'function' || cc.className === 'function')?.contains?.find(cc => cc.scope === 'params' || cc.className === 'params').variants[2];
Copy link
Member

@joshgoebel joshgoebel Nov 13, 2023

Choose a reason for hiding this comment

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

The function scope is deprecated and will be removed in the future... though I'm not sure this is necessary at all once you follow the other suggestions.

if (
contains
) {
const params = contains.find(cc => cc.scope === 'params' || cc.className === 'params');
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need params here or just access to params_contains? See the already exported tsLanguage.exports.PARAMS_CONTAINS instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, thanks for your review.
Yes, I want to access the params_contains.
I will modify the code now and commit the changes.

Copy link

Build Size Report

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

7 files changed

Total change +87 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +2 B
es/highlight.min.js 8.13 KB 8.14 KB +2 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
highlight.min.js 8.17 KB 8.17 KB +3 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

@@ -11,7 +11,7 @@ import * as ECMAScript from "./lib/ecmascript.js";
import javascript from "./javascript.js";

/** @type LanguageFn */
export default function(hljs) {
export default function (hljs) {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the linting/spacing only changes.

Copy link

Build Size Report

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

6 files changed

Total change +82 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +1 B
es/highlight.min.js 8.13 KB 8.14 KB +1 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

Copy link

Build Size Report

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

7 files changed

Total change +86 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +2 B
es/highlight.min.js 8.13 KB 8.14 KB +2 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
highlight.min.js 8.17 KB 8.17 KB +2 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

@joshgoebel
Copy link
Member

Now we just need a changelog entry I think.

@joshgoebel joshgoebel changed the title fix(typescript): params types enh(typescript): highlight obvious user defined type names (interface, etc) Nov 15, 2023
Copy link

Build Size Report

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

7 files changed

Total change +86 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +2 B
es/highlight.min.js 8.13 KB 8.14 KB +2 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
highlight.min.js 8.17 KB 8.17 KB +2 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

@joshgoebel
Copy link
Member

Looks like markup tests aren't all passing?

@MohamedAli00949
Copy link
Contributor Author

Looks like markup tests aren't all passing?

I tested them and all have been passed before creating the PR and after modification at Typescript and all passed.

If you have found any failed test comment it to fix it.
Thanks in advance

@joshgoebel
Copy link
Member

See the failure here: https://github.com/highlightjs/highlight.js/actions/runs/6872592218/job/18691291704?pr=3903

Are you saying they all pass for you locally? Is your working tree clean?

@MohamedAli00949
Copy link
Contributor Author

See the failure here: https://github.com/highlightjs/highlight.js/actions/runs/6872592218/job/18691291704?pr=3903

Are you saying they all pass for you locally? Is your working tree clean?

You are right, I am sorry.

This is happening because I am running an npm run test command to test all test types.
But the markup tests have passed.

Screenshot 2023-11-16 032743

And the result of npm run test-markup is the result at the above image.

Screenshot 2023-11-16 032521

I will commit changing at declares.expect.txt and jsx.expect.txt now.

Copy link

Build Size Report

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

7 files changed

Total change +83 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +1 B
es/highlight.min.js 8.13 KB 8.14 KB +1 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
highlight.min.js 8.17 KB 8.17 KB +1 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

Copy link

Build Size Report

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

7 files changed

Total change +86 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +2 B
es/highlight.min.js 8.13 KB 8.14 KB +2 B
es/languages/javascript.min.js 2.68 KB 2.69 KB +12 B
es/languages/typescript.min.js 3.08 KB 3.11 KB +31 B
highlight.min.js 8.17 KB 8.17 KB +2 B
languages/javascript.min.js 2.69 KB 2.7 KB +7 B
languages/typescript.min.js 3.09 KB 3.12 KB +30 B

@joshgoebel joshgoebel merged commit c668a45 into highlightjs:main Nov 16, 2023
13 of 14 checks passed
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