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

Doc comments highlight improvements + closing auto-insertion #5109

Merged
merged 6 commits into from
May 11, 2023

Conversation

mkslanc
Copy link
Contributor

@mkslanc mkslanc commented Mar 31, 2023

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Patch coverage: 90.13% and project coverage change: -0.01 ⚠️

Comparison is base (204aafa) 86.94% compared to head (536939d) 86.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5109      +/-   ##
==========================================
- Coverage   86.94%   86.94%   -0.01%     
==========================================
  Files         561      562       +1     
  Lines       44962    44968       +6     
  Branches     6913     6919       +6     
==========================================
+ Hits        39094    39099       +5     
- Misses       5868     5869       +1     
Flag Coverage Δ
unittests 86.94% <90.13%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/mode/swift.js 100.00% <ø> (ø)
src/mode/ion.js 74.07% <74.07%> (-0.93%) ⬇️
src/mode/partiql.js 74.07% <74.07%> (-0.93%) ⬇️
src/mode/behaviour/cstyle.js 89.80% <93.33%> (+0.17%) ⬆️
src/mode/apex.js 100.00% <100.00%> (ø)
src/mode/asl.js 100.00% <100.00%> (ø)
src/mode/behaviour/behaviour_test.js 99.42% <100.00%> (+0.02%) ⬆️
src/mode/c_cpp.js 51.21% <100.00%> (-2.27%) ⬇️
src/mode/crystal.js 46.51% <100.00%> (-1.22%) ⬇️
src/mode/csharp.js 66.66% <100.00%> (-0.99%) ⬇️
... and 32 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkslanc mkslanc marked this pull request as ready for review April 4, 2023 10:18
src/mode/c_cpp.js Outdated Show resolved Hide resolved
var CStyleFoldMode = require("./folding/cstyle").FoldMode;

var Mode = function() {
this.HighlightRules = c_cppHighlightRules;

this.$outdent = new MatchingBraceOutdent();
this.$behaviour = new CstyleBehaviour();
this.$behaviour = new CstyleBehaviour({closeDocComment: true});
Copy link
Member

Choose a reason for hiding this comment

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

where do we need this to be false? maybe it would be better to make it true by default?

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 am not sure if this would be the right approach. For example, Python uses this.$behaviour = this.$defaultBehaviour;, which is essentially a generic CStyleBehaviour. However, Python itself employs a different syntax for docstrings, without using /**. It would work as expected because I check if the state is doc-start, but I am not sure if making closeDocComment: true the default setting is the correct approach in general.

},
"start": [
{
token: ["comment.doc.tag", "text", "lparen.doc"],
Copy link
Member

Choose a reason for hiding this comment

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

should this be in generic doc_comment highlight rules or specific to js and ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this seems should be specific to js and ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But also most of languages doesn't have specific tags starting with @ , which were in DocCommentHighlightRules before I changed it, what should I do with it?

@mkslanc mkslanc requested a review from nightwing May 3, 2023 12:22

var Mode = function() {

this.HighlightRules = EdifactHighlightRules;
this.$behaviour = new CstyleBehaviour();
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need these instead of relying on default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could rely on default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced all new CstyleBehaviour() to default ones

@mkslanc mkslanc requested a review from nightwing May 4, 2023 13:17
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