-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
Support multiple setting sets per file #1156
Support multiple setting sets per file #1156
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Would be nice to test it with scripted tests.
ebf6845
to
e5a6f46
Compare
calibanSetting(file("src/main/graphql/schema.graphql"))( // Explicitly constrain to disambiguate | ||
cs => | ||
cs.clientName("Client") | ||
calibanSetting(file("src/main/graphql/schema.graphql"))( // Explicitly constrain to disambiguate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I don't get what this "constrains", is that comment up to date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not the most helpful comment, as I don't understand what needed to be disambiguated here -- perhaps back when it was possible (or intended) to star-glob multiple files? I could see trying to disambiguate between the few .graphql
test specifications we had at that time.
@ghostdogpr updated with a test and some other changes (see note in PR description) :) cc @guizmaii I'd appreciate some eyes on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from me, good addition.
calibanSetting(file("src/main/graphql/schema.graphql"))( // Explicitly constrain to disambiguate | ||
cs => | ||
cs.clientName("Client") | ||
calibanSetting(file("src/main/graphql/schema.graphql"))( // Explicitly constrain to disambiguate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not the most helpful comment, as I don't understand what needed to be disambiguated here -- perhaps back when it was possible (or intended) to star-glob multiple files? I could see trying to disambiguate between the few .graphql
test specifications we had at that time.
) | ||
val matchingSettingSets = fileSettings.collect { | ||
case needle if source.toPath.endsWith(needle.file.toPath) => needle.settings | ||
}.map(defaults.combine(_)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the intended change in this PR, from what I can tell. I was foldLeft
-ing in the beginning to permit tiered configuration, but that was before the cs
pattern was discovered. I don't think it's useful to accumulate anymore, if the previous implementation was desired, it is possible to define a generic CalibanSettings => CalibanSettings
and apply that before you chain the rest of your config.
vuepress/docs/docs/client.md
Outdated
Compile / caliban / calibanSources := file("caliban") | ||
``` | ||
|
||
If you want to only include certain files in a directory, you can override that as well with an explicit `caliban / sources` entry: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually true, since calibanSources
is a File
, not a List[File]
-- the comment doesn't scale and will trip up users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, hopefully it's correct now
LGTM, I will merge! |
This would be useful in situations where you want to generate both a server and a client from a single schema file, with potentially different settings.
Update: Added some more changes:
calibanGenClient
calls and one for the setting-based generation