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

fix Custom Scalar should not be edited. output #243

Merged

Conversation

matsudamper
Copy link
Contributor

@matsudamper matsudamper commented Jan 22, 2024

fix: apollographql/apollo-ios#3323

cast to TemplateRenderer

var template: TemplateRenderer {
CustomScalarTemplate(graphqlScalar: graphqlScalar, config: config)
}

Optional is different.

func renderBodyTemplate(
nonFatalErrorRecorder: ApolloCodegen.NonFatalError.Recorder
) -> TemplateString {

The implementation of the extension is used.

func renderHeaderTemplate(
nonFatalErrorRecorder: ApolloCodegen.NonFatalError.Recorder
) -> TemplateString? {
TemplateString(HeaderCommentTemplate.template.description)
}

relevance
#152

@matsudamper matsudamper requested a review from a team as a code owner January 22, 2024 13:00
@apollo-cla
Copy link

@matsudamper: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link

netlify bot commented Jan 22, 2024

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 61a5f56

@matsudamper matsudamper changed the title fix not used implementation fix Custom Scalar should not be edited. output Jan 22, 2024
@matsudamper matsudamper changed the title fix Custom Scalar should not be edited. output fix Custom Scalar should not be edited. output Jan 22, 2024
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Thanks so much for this bug fix! This test was missing and made this bug difficult to catch during my refactor.

@AnthonyMDev AnthonyMDev enabled auto-merge (squash) January 22, 2024 21:47
@AnthonyMDev AnthonyMDev merged commit ebccdfa into apollographql:main Jan 22, 2024
14 checks passed
BobaFetters pushed a commit that referenced this pull request Jan 22, 2024
BobaFetters pushed a commit to apollographql/apollo-ios-codegen that referenced this pull request Jan 22, 2024
BobaFetters pushed a commit that referenced this pull request Jan 22, 2024
99aa12d9 fix Custom Scalar  output (#243)

git-subtree-dir: apollo-ios-codegen
git-subtree-split: 99aa12d9c1be923c3c30e8cb54babdf7c574d96b
BobaFetters pushed a commit that referenced this pull request Jan 22, 2024
git-subtree-dir: apollo-ios-codegen
git-subtree-mainline: 1ebbe4c
git-subtree-split: 99aa12d9c1be923c3c30e8cb54babdf7c574d96b
@calvincestari
Copy link
Member

Do we need to be worried that this was more than just a comment-related bug? I'd be far more concerned if we were actually overwriting pre-existing custom scalar files; do we need an additional test for that?

@AnthonyMDev
Copy link
Contributor

AnthonyMDev commented Jan 22, 2024

No, this wouldn't have affected that. It was just a change in the function signature for the functions:

func renderHeaderTemplate(
     nonFatalErrorRecorder: ApolloCodegen.NonFatalError.Recorder
   ) -> TemplateString? {

func renderDetachedTemplate(
    nonFatalErrorRecorder: ApolloCodegen.NonFatalError.Recorder
  ) -> TemplateString? {
    nil
  }

and

func renderBodyTemplate(
    nonFatalErrorRecorder: ApolloCodegen.NonFatalError.Recorder
  ) -> TemplateString

It would not affect the logic for the overwriting.

@matsudamper matsudamper deleted the fix-scalar-not-edited-change branch January 22, 2024 22:42
@calvincestari
Copy link
Member

Do we need to be worried that this was more than just a comment-related bug? I'd be far more concerned if we were actually overwriting pre-existing custom scalar files; do we need an additional test for that?

Executing the run-codegen.sh script on the 1.8.0 tagged release doesn't change any of the generated files and we have generated custom scalars in both AnimalKingdomAPI and GitHubAPI.

I've had a look through FileGeneratorTests and ApolloCodegenTests but I don't think we have a test that actually checks written file output to ensure nothing is written to pre-existing files that should not be overwritten.

@calvincestari
Copy link
Member

No, this wouldn't have affected that. It was just a change in the function signature for the functions:

Sorry. I had my reply in draft for a while and didn't see your reply before posting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants