Skip to content

Feature/generator params#21

Merged
markalfred merged 4 commits intomarkalfred:masterfrom
AshesOfOwls:feature/generator_params
Nov 29, 2017
Merged

Feature/generator params#21
markalfred merged 4 commits intomarkalfred:masterfrom
AshesOfOwls:feature/generator_params

Conversation

@AshesOfOwls
Copy link
Contributor

@AshesOfOwls AshesOfOwls commented Nov 16, 2017

  • Passes in propName and isRequired params to all generator functions.
  • Converts default props to reflect their propName where applicable.
  • Updates tests to reflect new default values.
  • Adds .DS_Store to the gitignore file.

Closes #20


const generateOneProp = (propType, propName) => {
const generate = options.generators[propType.type]
const generateOneProp = (propType, propName, wrapInArray=true) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapInArray may be better off with a different name but I was struggling to find one.

Copy link
Owner

Choose a reason for hiding this comment

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

wrapInArray might be something like asPair or more verbosely asKeyValuePair — it was always just used in the .fromPairs call. wrapInArray is describing what it's doing though, so I think it's cool. This makes me think that I might want a more robust way for the primary generateProps call to invoke this function and get the structure that it needs back. In other words, let generateProps handle massaging the data into a form that it likes — don't put that responsibility on generateOneProp. I'll take this on in my next update 👍

Copy link
Owner

@markalfred markalfred left a comment

Choose a reason for hiding this comment

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

Thank you @mcpants ! This is excellent, I have just one question about isRequired but this looks good to go. Once shipped, I'll update the readme and cut a new version. Thanks again for your contribution 🙇


const generateOneProp = (propType, propName) => {
const generate = options.generators[propType.type]
const generateOneProp = (propType, propName, wrapInArray=true) => {
Copy link
Owner

Choose a reason for hiding this comment

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

wrapInArray might be something like asPair or more verbosely asKeyValuePair — it was always just used in the .fromPairs call. wrapInArray is describing what it's doing though, so I think it's cool. This makes me think that I might want a more robust way for the primary generateProps call to invoke this function and get the structure that it needs back. In other words, let generateProps handle massaging the data into a form that it likes — don't put that responsibility on generateOneProp. I'll take this on in my next update 👍

src/main.js Outdated
const forceGenerateOneProp = (propType) => {
const generate = GENERATORS[propType.type]
const forceGenerateOneProp = (propType, propName) => {
const generate = GENERATORS[propType.type].bind(this, propName, !propType.isRequired)
Copy link
Owner

Choose a reason for hiding this comment

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

What function is the isRequired argument serving here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was out of novelty, I figured there might be a use to getting this information to the user. In light of the changes that you discussed in your previous comment, I am going to strip this out so that this PR has less overhead. If you decide it would be useful in the future then it can easily be added back in.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahhhhh okay, makes sense. Thank you for the explanation, if we find a good usecase for it in the future I'm open to the idea 👍

@AshesOfOwls
Copy link
Contributor Author

Sorry for the delay, will get back to this asap :)


.env
dist
.DS_Store
Copy link
Owner

Choose a reason for hiding this comment

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

😳 😳 😳 👏 👏 👏

@AshesOfOwls
Copy link
Contributor Author

All updated!

@markalfred
Copy link
Owner

Looking excellent, thank you for the contribution @mcpants ! 🙇

@markalfred markalfred merged commit c29c3b6 into markalfred:master Nov 29, 2017
@AshesOfOwls
Copy link
Contributor Author

🎊

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