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: Customized Prefixing #23

Closed
wants to merge 4 commits into from
Closed

Feat: Customized Prefixing #23

wants to merge 4 commits into from

Conversation

3nvi
Copy link
Contributor

@3nvi 3nvi commented Jul 13, 2020

This PR simply provides an option for users to specify the prefix that their mock builders will have.

Closes #22

Additional Context

Initially I tried to implement the addPrefix option, but I saw that when omitting the prefix, the generator function names collided with the name of the existing TS types from @graphql-codegen/typescript. That's why a prefix must always be present.

This PR makes sure to fallback to the default prefix implementation even if the user manually specifies '' as a prefix just for this reason alone

@3nvi 3nvi changed the title Feat: Optional Prefixing Feat: Customized Prefixing Jul 13, 2020
@3nvi
Merge remote-tracking branch 'origin/master' into feat-optional-prefi…
cb7ebf8 
…xing

# Conflicts:
#	src/index.ts
#	tests/typescript-mock-data.spec.ts
ardeois
ardeois reviewed Jul 13, 2020
View reviewed changes
Copy link
Owner

@ardeois ardeois left a comment

Choose a reason for hiding this comment

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

Very nice thanks
For the scalar, I totally agree, but for the prefix I was thinking of using indefinite package instead, so we have proper a vs an prefix.
Would that fix your problems?

@3nvi
Copy link
Contributor Author

3nvi commented Jul 13, 2020

It would yeah, although I feel that the PR is just related to the bug itself, but to a new feature as well. In any case, I'm fully ok if you don't want to merge this due to the fact that it might make things more complex

@ardeois
Copy link
Owner

ardeois commented Jul 13, 2020 •
edited
Loading

Well it seems like this PR includes the scalar changes as well, I think you have an issue with your branches.
And yes to be honest, I would prefer to use indefinite instead of having custom prefix. But this is a personal opinion and I don't see why other developers might not decide differently.
So I'm not against merging this, however I think you should make sure this PR only contains the changes for the prefix. The custom scalar should be a different branch and PR #21

@3nvi
Copy link
Contributor Author

3nvi commented Jul 13, 2020

Well it seems like this PR includes the scalar changes as well, I think you have an issue with your branches.
And yes to be honest, I would prefer to use indefinite instead of having custom prefix. But this is a personal opinion and I don't see why other developers might not decide differently.
So I'm not against merging this, however I think you should make sure this PR only contains the changes for the prefix. The custom scalar should be a different branch and PR #21

Yeah you are right. I wanted to experiment a bit and I made a wrong merge. I'll clean all this up and start over

@3nvi
Copy link
Contributor Author

3nvi commented Jul 13, 2020

Closing in favor of #25

@3nvi 3nvi closed this Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardeois ardeois ardeois left review comments

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

Successfully merging this pull request may close these issues.

Feature Request: allow specification/omit of the prefix
2 participants
@3nvi @ardeois