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

docs: check and improve handling of duplicate tags #2444

Merged
merged 25 commits into from Dec 3, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 7, 2023

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: infra Changes to our infrastructure or project setup labels Oct 7, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 7, 2023
@ST-DDT ST-DDT requested review from a team October 7, 2023 14:03
@ST-DDT ST-DDT self-assigned this Oct 7, 2023
@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #2444 (dbe9739) into next (9dd57d7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2444   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files        2805     2805           
  Lines      250168   250168           
  Branches     1106     1107    +1     
=======================================
+ Hits       249118   249120    +2     
+ Misses       1022     1020    -2     
  Partials       28       28           
Files Coverage Δ
src/modules/helpers/index.ts 99.01% <100.00%> (ø)

... and 1 file with indirect coverage changes

@matthewmayer
Copy link
Contributor

This is a behavior change no? The default for join is ,

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 8, 2023

Technically yes. Practically not, because we only ever have one.

@matthewmayer
Copy link
Contributor

If we did have more than one should that throw an error?

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 8, 2023

If we did have more than one should that throw an error?

Changed. It looks like there was one method with two @throws tags, that I have missed in my earlier checks.

@matthewmayer
Copy link
Contributor

This PR should probably be renamed now then

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 8, 2023

This PR should probably be renamed now then

The title is already very long so appending it might be hard.
Do you have a suggestion?

@matthewmayer
Copy link
Contributor

Split the changes about allowing one one of each tag to a seperate PR?

@ST-DDT ST-DDT changed the title infra(unicorn): require-array-join-separator docs: check and improve handling of duplicate tags Oct 9, 2023
@ST-DDT ST-DDT added the c: docs Improvements or additions to documentation label Oct 9, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 9, 2023

IMO this only does a single thing, what should I split here?

I renamed it to reflect its doc impact

@xDivisionByZerox
Copy link
Member

Preview of the changes on multiple @throw tags:

image


I don't know if I'm the biggest fan as you can't tell right now that these are 2 distinct throw situations. But it is better than not having them at all.

@fisker
Copy link

fisker commented Oct 26, 2023

This end up with nothing related to require-array-join-separator at all... LOL

require-array-join-separator mean to make code easier for beginners and for consistency.

@xDivisionByZerox
Copy link
Member

This end up with nothing related to require-array-join-separator at all... LOL

require-array-join-separator mean to make code easier for beginners and for consistency.

True. Enabling the rule in this PR is kind of missleading.

.eslintrc.js Outdated Show resolved Hide resolved
@ST-DDT ST-DDT removed the c: infra Changes to our infrastructure or project setup label Oct 26, 2023
@ST-DDT ST-DDT merged commit 505f659 into next Dec 3, 2023
20 checks passed
@ST-DDT ST-DDT deleted the infra/unicorn/require-array-join-separator branch December 3, 2023 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants