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.

Sign up for GitHub

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

Jump to bottom

feat: show derived configuration #517

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: show derived configuration #517

wants to merge 1 commit into from

Conversation

mcknasty
Copy link
Contributor

@mcknasty mcknasty commented Jan 20, 2024 •
edited

Pull Request 517

Commit Message

commit b33e8e7
Author: Trevor D McKeown tmckeown@gmail.com
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 mcknasty marked this pull request as ready for review January 20, 2024 05:47
@mcknasty mcknasty mentioned this pull request Jan 20, 2024
Closed
12 tasks
@mcknasty mcknasty marked this pull request as draft January 20, 2024 20:21
@mcknasty
Copy link
Contributor Author

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.

mcknasty added a commit to mcknasty/c8 that referenced this pull request Jan 20, 2024
@mcknasty
Print Derived Configuration Report (bcoe#517)
e1969b0 
feat: print derived config variables
feat: print derived config variables in json
test: 12 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param
mcknasty added a commit to mcknasty/c8 that referenced this pull request Jan 20, 2024
@mcknasty
Print Derived Configuration Report (bcoe#517)
a09fd9d 
feat: print derived config variables
feat: print derived config variables in json
test: 12 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param
mcknasty added a commit to mcknasty/c8 that referenced this pull request Jan 20, 2024
@mcknasty
Print Derived Configuration Report (bcoe#517)
32dd6af 
feat: print derived config variables
feat: print derived config variables in json
test: 12 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param
@mcknasty mcknasty marked this pull request as ready for review January 20, 2024 23:50
bcoe
bcoe reviewed Jan 23, 2024
View reviewed changes
c8-ascii-art.txt Outdated Show resolved Hide resolved
bcoe
bcoe reviewed Jan 23, 2024
View reviewed changes
Copy link
Owner

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

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

feat: show derived configuration

@mcknasty mcknasty changed the title Print Derived Configuration Report feat: show derived configuration Jan 25, 2024
@mcknasty
Copy link
Contributor Author

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

mcknasty added a commit to mcknasty/c8 that referenced this pull request Feb 5, 2024
@mcknasty
Print Derived Configuration Report (bcoe#517)
e08d186 
feat: print derived config variables
feat: print derived config variables in json
test: 12 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param
mcknasty added a commit to mcknasty/c8 that referenced this pull request Feb 10, 2024
@mcknasty
Print Derived Configuration Report (bcoe#517)
20d6557 
feat: print derived config variables
feat: print derived config variables in json
test: 12 new unit tests to support features
test: 1 skipped unit test for discovered bug in yargs with reports param
@mcknasty
Print Derived Configuration Report (bcoe#517)
b33e8e7 
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
@mcknasty
Copy link
Contributor Author

@bcoe, Please review again.

@mcknasty mcknasty requested a review from bcoe February 18, 2024 14:45
bcoe
bcoe reviewed Feb 19, 2024
View reviewed changes
lib/print-config.js
console.log(jsonYargs)
}

// DO NOT REMOVE! This is intentional.
Copy link
Owner

Choose a reason for hiding this comment

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

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

Why is this process.exit() necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
bcoe reviewed Feb 19, 2024
View reviewed changes
test/test-helpers.js
/**
* Function: runSpawn
*
* @param {String} cmd: A string representing the prompt command
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mcknasty @bcoe