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

test(create-gatsby): snapshot plugins-options-form and tiny refactoring #28366

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

mfrachet
Copy link
Contributor

Description

Snapshotting output of makePluginConfigQuestions and add a tiny refacto

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 30, 2020
@mfrachet mfrachet removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 30, 2020
@mfrachet mfrachet requested a review from ascorbic November 30, 2020 11:17
@mfrachet mfrachet force-pushed the mfrachet/plugins-options-form-tests branch from 95b72f8 to 43106ca Compare November 30, 2020 11:18
gillkyle
gillkyle previously approved these changes Nov 30, 2020
Copy link
Contributor

@gillkyle gillkyle left a comment

Choose a reason for hiding this comment

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

I like reading through these to see what you change. This one looks pretty straightforward to me, testing the output of makePluginConfigQuestions so we don't do something that breaks what gets piped into the enquirer form 👍

@mfrachet
Copy link
Contributor Author

mfrachet commented Dec 1, 2020

Thanks for the feedback 😊 I (we actually 😬 ) tried to test from the enquirer standpoint and got in trouble when going in the CI. We then decided to step back and just test the different functions to make sure everything looks okay 😊

ascorbic
ascorbic previously approved these changes Dec 1, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple of typos

const schemaToChoice = ([name, option]: [string, Schema]): IChoice => {
const hasDefaultValue =
option.flags?.default &&
supportedOptionTypes.includes(typeof option.flags?.default)
Copy link
Contributor

Choose a reason for hiding this comment

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

I realise this was how it was before, but I do wonder if there's a better way to do this, using the schema rather than checking the type of the default value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a side note on that: right now, IIRC, none of the elements that are required have a default value 🤔 . This is a computation that lives there "for nothing" for now I would say :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Most of them aren't likely to either, because if there's a default it won't need to be required

@mfrachet mfrachet dismissed stale reviews from ascorbic and gillkyle via 10bbc17 December 1, 2020 09:35
@mfrachet mfrachet requested review from ascorbic and gillkyle December 1, 2020 09:37
@mfrachet mfrachet merged commit a19a76d into master Dec 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the mfrachet/plugins-options-form-tests branch December 1, 2020 11:06
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.

3 participants