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: improve finance jsdocs #434

Merged
merged 3 commits into from
Feb 6, 2022
Merged

docs: improve finance jsdocs #434

merged 3 commits into from
Feb 6, 2022

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Feb 5, 2022

Ref: #203

@ST-DDT ST-DDT added the c: docs Improvements or additions to documentation label Feb 5, 2022
@ST-DDT ST-DDT requested a review from a team February 5, 2022 13:44
@ST-DDT ST-DDT self-assigned this Feb 5, 2022
@codecov
Copy link

codecov bot commented Feb 5, 2022

Codecov Report

Merging #434 (46efb30) into main (3ec32a3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #434   +/-   ##
=======================================
  Coverage   99.32%   99.32%           
=======================================
  Files        1920     1920           
  Lines      174047   174086   +39     
  Branches      895      895           
=======================================
+ Hits       172876   172916   +40     
+ Misses       1115     1114    -1     
  Partials       56       56           
Impacted Files Coverage Δ
src/definitions/finance.ts 100.00% <100.00%> (ø)
src/finance.ts 99.33% <100.00%> (+0.06%) ⬆️
src/helpers.ts 99.47% <0.00%> (+0.17%) ⬆️

@Shinigami92
Copy link
Member

bcoe/c8#182

@bcoe @j03m
/cc @antfu

Did we misc-configured something in our vite.config.ts

faker/vite.config.ts

Lines 1 to 14 in 832bf8a

/// <reference types="vitest" />
import { defineConfig } from 'vite';
// https://vitejs.dev/config/
export default defineConfig({
test: {
coverage: {
all: true,
reporter: ['clover', 'cobertura', 'lcov', 'text'],
// TODO christopher 2022-02-04: Later we will only cover `src` instead of `dist` and `src`
include: ['dist', 'src'],
},
},
});

Or is it really a bug by c8 that JSDoc multi-line comments are count as needs to be covered by tests? 👀
If so, this could be a huge bummer for vitest 😬

@bcoe
Copy link

bcoe commented Feb 5, 2022

@Shinigami92 I would take a look at the .js generated from the .ts file, sometimes there's an unexpected code path that gets generated, by virtue of the structure of the generated code.

@Shinigami92
Copy link
Member

@bcoe So we already want to switch to just cover the src folder instead of the dist/cjs (#432)
Do you think this could already fix it?

@bcoe
Copy link

bcoe commented Feb 5, 2022

Do you think this could already fix it?

There's a chance. The problem is generally a disconnect between the .ts and the .js, if the typescript compiler introduces an extra logical expression that's not exercised, or the source map remapping has a hiccup, there might be an uncovered element.

@ST-DDT ST-DDT merged commit aae13b3 into main Feb 6, 2022
@ST-DDT ST-DDT deleted the jsdocs/finance branch February 6, 2022 10:40
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants