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(jsdocs): solve all lint warnings in project #869

Closed
wants to merge 29 commits into from

Conversation

rinaok
Copy link
Contributor

@rinaok rinaok commented Nov 29, 2022

Waiting for #110

@rinaok rinaok self-assigned this Nov 29, 2022
@rinaok rinaok added the Type: Fix small fix or a bug fix label Nov 29, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #869 (c451ea8) into main (d61b119) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #869    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          123       194    +71     
  Lines         1562      2149   +587     
  Branches       108       130    +22     
==========================================
+ Hits          1562      2149   +587     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
...omponents/src/lib/accordion-item/accordion-item.ts 100.00% <ø> (ø)
...components/src/lib/accordion/accordion.template.ts 100.00% <ø> (ø)
libs/components/src/lib/accordion/accordion.ts 100.00% <ø> (ø)
...ents/src/lib/action-group/action-group.template.ts 100.00% <ø> (ø)
...bs/components/src/lib/action-group/action-group.ts 100.00% <ø> (ø)
libs/components/src/lib/badge/badge.ts 100.00% <ø> (ø)
libs/components/src/lib/banner/banner.ts 100.00% <ø> (ø)
...ponents/src/lib/breadcrumb-item/breadcrumb-item.ts 100.00% <ø> (ø)
...mponents/src/lib/breadcrumb/breadcrumb.template.ts 100.00% <ø> (ø)
libs/components/src/lib/breadcrumb/breadcrumb.ts 100.00% <ø> (ø)
... and 178 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

please await #861 as this is a major change applying many files

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

some of the ommited decorators like accessors (e.g. @public) are TSDoc related. we might need to go over them all and fix due to #818

@rinaok
Copy link
Contributor Author

rinaok commented Dec 5, 2022

please await #861 as this is a major change applying many files

@yinonov Now after #861 is merged, can we merge this?

@rinaok rinaok requested a review from yinonov December 5, 2022 11:48
@yinonov
Copy link
Contributor

yinonov commented Dec 5, 2022

please await #861 as this is a major change applying many files

@yinonov Now after #861 is merged, can we merge this?

it requires a very thorough review still. the pain point with this symptom relates to #110 (comment)

the optimal solution is, if tsdoc-lint was to enforce comments, to remove jsdoc-lint and use the tsdoc-lint instead

@rinaok
Copy link
Contributor Author

rinaok commented Dec 5, 2022

please await #861 as this is a major change applying many files

@yinonov Now after #861 is merged, can we merge this?

it requires a very thorough review still. the pain point with this symptom relates to #110 (comment)

the optimal solution is, if tsdoc-lint was to enforce comments, to remove jsdoc-lint and use the tsdoc-lint instead

@yinonov But you said that we should avoid at this point from tsdoc #110 (comment)

@yinonov
Copy link
Contributor

yinonov commented Dec 6, 2022

Because of lint technical limitation not enforcing necessary comments. But if u already went through and added all, we can reconsider tsdoc lint.
It will do more, now that api-extractor generate meta data

Copy link
Contributor

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

I think we can exempt test files (.spec|test.ts) from tsdoc linting rule.
What do you think @olaf-k ?

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

As #110 expected to now redfine eslint rules, it might outdate the changes of this PR.

what worries me is the fact we are not that acquainted with TSDoc and how to use it to its extent

the general idea to automate and improve our workflow is to have comments populating to READMEs and from there to docs website

for web components interfaces, TSDoc lacks some features, of where custom-elements-manifests come into play.

1st, let's make 1, single component as perfect POC, have its doc generated automatically.

going over lots of files to fix without a solid understanding the result will probably buy us more debts

@olaf-k
Copy link
Contributor

olaf-k commented Dec 21, 2022

I think we can exempt test files (.spec|test.ts) from tsdoc linting rule. What do you think @olaf-k ?

my gut feeling would be "yes please!" or at least keep only rules that prevent readability issues (weird capitalization, mix tab/space, this sort of stuff)

@yinonov
Copy link
Contributor

yinonov commented Dec 21, 2022

I think we can exempt test files (.spec|test.ts) from tsdoc linting rule. What do you think @olaf-k ?

my gut feeling would be "yes please!" or at least keep only rules that prevent readability issues (weird capitalization, mix tab/space, this sort of stuff)

aware of any such preceding practice?

@olaf-k
Copy link
Contributor

olaf-k commented Dec 21, 2022

aware of any such preceding practice?

following our chat: it makes sense to keep some formatting rules to keep the code clean and readable for team members and minimize commit noise, while removing strict rules (e.g., no empty functions) for test files.

Copy link
Contributor

@olaf-k olaf-k left a comment

Choose a reason for hiding this comment

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

Note that the default linting target uses nx's run-many to check all projects and thus skips warnings.

To get the warnings displayed you must either add --verbose to the linting target command or simple run it project by project (e.g., npx nx lint components).

At the time of writing, there are still 400+ warning messages from the components project, repeating ones from tsdoc complaining about jsdoc formatting.

@rinaok
Copy link
Contributor Author

rinaok commented Dec 26, 2022

Note that the default linting target uses nx's run-many to check all projects and thus skips warnings.

To get the warnings displayed you must either add --verbose to the linting target command or simple run it project by project (e.g., npx nx lint components).

At the time of writing, there are still 400+ warning messages from the components project, repeating ones from tsdoc complaining about jsdoc formatting.

@olaf-k good catch! @YonatanKra

Copy link
Contributor

@olaf-k olaf-k left a comment

Choose a reason for hiding this comment

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

Seeing the diff, there's a bunch of comments that could use some cleanup, but that's beyond the scope of this PR.

@rinaok rinaok marked this pull request as draft January 3, 2023 10:36
@rinaok rinaok closed this Mar 22, 2023
@rinaok rinaok deleted the fix(lint)--warnings branch July 10, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix small fix or a bug fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants