feat: show derived configuration

[mcknasty]

Pull Request 517

Commit Message

commit b33e8e7
Author: Trevor D McKeown [email protected]
Date: Sun Feb 18 09:17:58 2024 -0500

Print Derived Configuration Report (#517)

feat: print derived config variables
feat: print derived config variables in json
test: 16 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param

Following the Contributions Recommendations here.

1. Make sure you have installed the latest version of Node.js
2. Fork this project on GitHub and clone your fork locally
3. Create local branches to work within. These should also be created directly from the main branch. Local Fork here.
4. Make your changes.
5. Run tests to make sure all is okay (everything should pass except the snapshot).
   1. A complete log of initial test results.
   2. As instructed, ignore snapshot failures. 1 pending
   3. 111 passing in 54 seconds
6. Now update the snapshot.
   1. A complete log of snapshot test results.
   2. 111 passing in 54 seconds
7. If all is passing, commit your changes. The log of the commit can be found here.
8. As a best practice, once you have committed your changes, it is a good idea to use git rebase (not git merge) to synchronize your work with the main repository.
9. Run tests again to make sure all is okay.
   1. A complete log of the final test results.
   2. 111 passing in 55 seconds
10. Push
11. Open the pull request, see details in the template.
12. Make any necessary changes after review.

[mcknasty]

Saw a few nit picks. I might be able to refactor this patch to use fewer files, by naming the snapshots in the test cases. WIP.

[bcoe]

Looks like a useful addition to functionality to me, I would change the PR title to:

feat: show derived configuration

[mcknasty]

Thanks @bcoe. I will make the changes above and have a new commit in the next week.

[mcknasty]

@bcoe, Please review again.

[bcoe]

Could you expand on the comment to explain why this process.exit() is necessary?

Why is this process.exit() necessary?

[mcknasty]

The reason for this process.exit() is that it stops c8 from producing a coverage report after showing the configuration report. Additionally, this logic allows JSON to be also printed, and possibly consumed by another process.

@bcoe can you anticipate any use cases where a configuration report and coverage report are necessary?

[bcoe]

This seems like quite a bit of test helper logic being introduced for the size of the feature.

Could we get away with simply using a regex assertion and testing against some of the expected output?

[bcoe]

@mcknasty it's mostly my fault for not being more engaged on this repo, but I think I need to declare bankruptcy on some of the inflight PRs.

If you're still feeling passionate about some of these changes (some were pretty cool IMO, like printing derived configuration), mind reopening on at a time, with a tracking issue that describes the functionality you'd like to see.

Thank you for the work you have done on this repo 👏

[mcknasty]

Thanks for your comments on these. Some of the PRs did need to get closed. Unfortunately, this one is a prerequisite for #518 at the moment.