Skip to content
This repository was archived by the owner on Dec 8, 2021. It is now read-only.

Use the defaultName pattern for suggested model type names. #237

Merged

Conversation

stephenh
Copy link
Contributor

@stephenh stephenh commented Nov 2, 2018

There is technically an assumption in here that we should use the 1st file's defaultName, when the user might have N files with different defaultNames, but I think this will be what the user wants most of the time.

Fixes #226.

@Weakky
Copy link
Contributor

Weakky commented Nov 2, 2018

Hey!

Thanks for your work again! I think we need to change the design a bit regarding this feature.

Always using the first one seems really arbitrary and will lead to other problems.
Once you have several files with different defaultName, choosing the first one doesn't make any sense anymore.

My suggestion is the following:

Use the defaultName to name the suggested models when all the files have the same defaultName, otherwise, fallback to regular name.

If there's only one file with a defaultName, it should work as well.

What do you think?

@stephenh
Copy link
Contributor Author

stephenh commented Nov 2, 2018

@Weakky sure, that sounds like a reasonable heuristic. I'll update to do that.

@stephenh stephenh force-pushed the use-default-name-in-step-1-output branch from 778cc12 to 9f37315 Compare November 2, 2018 16:08
@stephenh
Copy link
Contributor Author

stephenh commented Nov 2, 2018

I noticed the conflicts; will rebase. Also, per the design-decision flag, fwiw my use case/workflow for wanting this was running graphqlgen on an existing schema and looking at "...well, now I have a whole bunch of names to fix". :-)

(Also while talking about step 1 in general, I think it'd be cool if graphqlgen just wrote the types to where they are supposed to go. As if there is >more than a single console of output show, the copy/paste is somewhat tedious. Granted, historically I was verify hesitant about code generators writing to files that are also programmer-maintained, in case of accidental overwrites, but it seems like jest's inline snapshot feature is perfectly happy to do that and people, even myself somewhat surprisingly, have been generally accepting of that workflow. Not a big deal, but FWIW. I can/should file as a separate issue too.)

@schickling
Copy link
Contributor

schickling commented Nov 3, 2018

@stephenh would you mind rebasing and resolve the conflicts? Let's merge then ✅

Re automatic model/type generation, let's discuss this over here: #181

@stephenh stephenh force-pushed the use-default-name-in-step-1-output branch from 9f37315 to 12b3f9e Compare November 3, 2018 21:03
@stephenh stephenh force-pushed the use-default-name-in-step-1-output branch from 12b3f9e to da263aa Compare November 3, 2018 21:05
@stephenh
Copy link
Contributor Author

stephenh commented Nov 3, 2018

@schickling okay, should be good.

@schickling schickling merged commit 2415a44 into prisma-labs:master Nov 4, 2018
@stephenh stephenh deleted the use-default-name-in-step-1-output branch November 4, 2018 20:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants