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

Re-org code (retry) #453

Merged
merged 5 commits into from
Sep 3, 2023
Merged

Re-org code (retry) #453

merged 5 commits into from
Sep 3, 2023

Conversation

jsumners
Copy link
Member

This is a replacement for #449 that GH closed and won't let me reopen.

The impetus for this PR was me starting to work on a suggestion PR for #445. When I got into the code, I decided that we have long ago outgrown the structure of the codebase as I created it when splitting this module out of the core Pino module. Basically, it has grown in a difficult to navigate mess with everything crammed into a few large files.

I have more that I intend to do, but I think this PR has become large enough as it is. So I'm halting my work until we make a decision on this (🤞 that means it's merged).


This PR is not yet complete as of commit 065fc99. I just want to put it up to show the following:

As of that commit, the "utils" file has been split out into a directory, lib/utils/. This directory has one file per utility function and a corresponding test file for each utility function file. It also updates the tap configuration to include a coverage map. The result is that we are not actually covering at 100% as we assumed:

Coverage Report
❯ tap
Suites:   ​27 passed​, ​27 of 27 completed​
Asserts:  ​​​453 passed​, ​of 453​
ERROR: Coverage for lines (95.48%) does not meet global threshold (100%)
ERROR: Coverage for functions (93.75%) does not meet global threshold (100%)
ERROR: Coverage for branches (85.77%) does not meet global threshold (100%)
ERROR: Coverage for statements (93.29%) does not meet global threshold (100%)
-------------------------------------|---------|----------|---------|---------|-------------------
File                                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------------------------|---------|----------|---------|---------|-------------------
All files                            |   93.29 |    85.77 |   93.75 |   95.48 |                   
 lib                                 |     100 |    90.62 |     100 |     100 |                   
  colors.js                          |     100 |    90.62 |     100 |     100 | 47-48,59          
 lib/utils                           |   92.19 |    84.97 |   91.66 |   94.71 |                   
  build-safe-sonic-boom.js           |   93.75 |    66.66 |   66.66 |   93.75 | 21                
  create-date.js                     |     100 |      100 |     100 |     100 |                   
  delete-log-property.js             |     100 |       80 |     100 |     100 | 24                
  filter-log.js                      |     100 |      100 |     100 |     100 |                   
  format-time.js                     |   95.23 |    93.33 |     100 |   95.23 | 45                
  get-property-value.js              |     100 |      100 |     100 |     100 |                   
  handle-custom-levels-names-opts.js |    92.3 |    71.42 |     100 |     100 | 18,24             
  handle-custom-levels-opts.js       |    92.3 |    71.42 |     100 |     100 | 18,24             
  interpret-conditionals.js          |     100 |      100 |     100 |     100 |                   
  is-object.js                       |     100 |      100 |     100 |     100 |                   
  is-valid-date.js                   |     100 |      100 |     100 |     100 |                   
  join-lines-with-indentation.js     |     100 |      100 |     100 |     100 |                   
  noop.js                            |     100 |      100 |     100 |     100 |                   
  prettify-error-log.js              |   38.46 |    46.15 |   33.33 |   45.45 | 48-69             
  prettify-error.js                  |     100 |      100 |     100 |     100 |                   
  prettify-level.js                  |     100 |    83.33 |     100 |     100 | 39                
  prettify-message.js                |     100 |      100 |     100 |     100 |                   
  prettify-metadata.js               |     100 |     87.5 |     100 |     100 | 28,32,43,50       
  prettify-object.js                 |   94.28 |    73.33 |     100 |     100 | 52,60,74-105      
  prettify-time.js                   |     100 |    91.66 |     100 |     100 | 45                
  split-property-key.js              |     100 |      100 |     100 |     100 |                   
-------------------------------------|---------|----------|---------|---------|-------------------

Okay, so that's not quite true. The difference is that some of the coverage comes through the factory function tests. With the restructure we have 1:1 test to code and we can see the gaps. it is my opinion that we should be able to recognize that file Y covers code X. Prior to this PR, one has to dig through a lot of code to find where something might be covering the code in question. As an example, in order to properly cover a case in handle-custom-levels-opts and handle-custom-levels-names-opts I had to read back through the git blame and learn what code was added to cover an odd branch.


After rebasing this on #451, I get numbers like this for the changes in this PR:

basicLog*10000: 759.376ms
objectLog*10000: 619.476ms
coloredLog*10000: 731.759ms
customPrettifiers*10000: 724.363ms
logWithErrorObject*10000: 561.199ms
logRemappedMsgErrKeys*10000: 556.772ms
messageFormatString*10000: 924.174ms
basicLog*10000: 726.8ms
objectLog*10000: 609.106ms
coloredLog*10000: 725.651ms
customPrettifiers*10000: 722.982ms
logWithErrorObject*10000: 562.306ms
logRemappedMsgErrKeys*10000: 561.282ms
messageFormatString*10000: 968.223ms

So, roughly no change in performance by splitting the functions out into their own files/scopes.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

rubberstamp lgtm

@jsumners jsumners merged commit b34e7c7 into master Sep 3, 2023
10 checks passed
@jsumners jsumners deleted the re-org-code branch September 3, 2023 11:29
renovate bot added a commit to fwouts/previewjs that referenced this pull request Oct 4, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [pino-pretty](https://togithub.com/pinojs/pino-pretty) | [`^10.2.0` ->
`^10.2.3`](https://renovatebot.com/diffs/npm/pino-pretty/10.2.0/10.2.3)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/pino-pretty/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/pino-pretty/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/pino-pretty/10.2.0/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/pino-pretty/10.2.0/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>pinojs/pino-pretty (pino-pretty)</summary>

###
[`v10.2.3`](https://togithub.com/pinojs/pino-pretty/releases/tag/v10.2.3)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/v10.2.2...v10.2.3)

#### What's Changed

- Manually require and export all util scripts by
[@&#8203;marvinruder](https://togithub.com/marvinruder) in
[pinojs/pino-pretty#467

#### New Contributors

- [@&#8203;marvinruder](https://togithub.com/marvinruder) made their
first contribution in
[pinojs/pino-pretty#467

**Full Changelog**:
pinojs/pino-pretty@v10.2.2...v10.2.3

###
[`v10.2.2`](https://togithub.com/pinojs/pino-pretty/releases/tag/v10.2.2)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/602be25a622812f8fbc2be318d4338a718c32902...v10.2.2)

#### What's Changed

- Add basic benchmark suite by
[@&#8203;jsumners](https://togithub.com/jsumners) in
[pinojs/pino-pretty#451
- Re-org code (retry) by
[@&#8203;jsumners](https://togithub.com/jsumners) in
[pinojs/pino-pretty#453
- Bump tsd from 0.28.1 to 0.29.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[pinojs/pino-pretty#454
- Bump actions/checkout from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[pinojs/pino-pretty#461
- Disable FinalizationRegistry if NODE_V8\_COVERAGE is set by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[pinojs/pino-pretty#464

**Full Changelog**:
pinojs/pino-pretty@v10.2.0...v10.2.2

###
[`v10.2.1`](https://togithub.com/pinojs/pino-pretty/compare/v10.2.0...602be25a622812f8fbc2be318d4338a718c32902)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/v10.2.0...602be25a622812f8fbc2be318d4338a718c32902)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/fwouts/previewjs).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjMiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjMiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
bodinsamuel pushed a commit to specfy/specfy that referenced this pull request Nov 3, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [pino-pretty](https://togithub.com/pinojs/pino-pretty) | [`10.2.0` ->
`10.2.3`](https://renovatebot.com/diffs/npm/pino-pretty/10.2.0/10.2.3) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/pino-pretty/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/pino-pretty/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/pino-pretty/10.2.0/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/pino-pretty/10.2.0/10.2.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>pinojs/pino-pretty (pino-pretty)</summary>

###
[`v10.2.3`](https://togithub.com/pinojs/pino-pretty/releases/tag/v10.2.3)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/v10.2.2...v10.2.3)

#### What's Changed

- Manually require and export all util scripts by
[@&#8203;marvinruder](https://togithub.com/marvinruder) in
[pinojs/pino-pretty#467

#### New Contributors

- [@&#8203;marvinruder](https://togithub.com/marvinruder) made their
first contribution in
[pinojs/pino-pretty#467

**Full Changelog**:
pinojs/pino-pretty@v10.2.2...v10.2.3

###
[`v10.2.2`](https://togithub.com/pinojs/pino-pretty/releases/tag/v10.2.2)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/602be25a622812f8fbc2be318d4338a718c32902...v10.2.2)

#### What's Changed

- Add basic benchmark suite by
[@&#8203;jsumners](https://togithub.com/jsumners) in
[pinojs/pino-pretty#451
- Re-org code (retry) by
[@&#8203;jsumners](https://togithub.com/jsumners) in
[pinojs/pino-pretty#453
- Bump tsd from 0.28.1 to 0.29.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[pinojs/pino-pretty#454
- Bump actions/checkout from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[pinojs/pino-pretty#461
- Disable FinalizationRegistry if NODE_V8\_COVERAGE is set by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[pinojs/pino-pretty#464

**Full Changelog**:
pinojs/pino-pretty@v10.2.0...v10.2.2

###
[`v10.2.1`](https://togithub.com/pinojs/pino-pretty/compare/v10.2.0...602be25a622812f8fbc2be318d4338a718c32902)

[Compare
Source](https://togithub.com/pinojs/pino-pretty/compare/v10.2.0...602be25a622812f8fbc2be318d4338a718c32902)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm every weekday" in timezone
Europe/Paris, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/specfy/specfy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6ImNob3JlL3Jlbm92YXRlQmFzZUJyYW5jaCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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