Zaid-Ajaj / Snowflaqe

A dotnet CLI to generate type-safe GraphQL clients for F# and Fable with automatic deserialization, static query verification and type checking
MIT License
157 stars 26 forks source link

Build props #45

Closed anthony-mi closed 3 years ago

anthony-mi commented 3 years ago

I have implemented generation props file with referenced fs files, but there was a problem while rewriting integration tests. The problem is that after the second execution of the command dotnet run (run -p Snowflaqe.fsproj -- --config ./snowflaqe-fsharp.json --generate) something is happening with Snowflaqe dependencies due to which the third run of the command and no longer creates the files necessary for the test.

xperiandri commented 3 years ago

Hi @anthony-mi I think there is a misunderstanding to the implementation. .props should not be the used instead of .fsproj but rather be an option/switch called propsFile which instructs Snowflaqe to generate .props file alongside the .fsproj

Why alongside? We either need .props or .fsproj. If we go with the switch we either produce .props or .fsproj. You won't ever need both.

Zaid-Ajaj commented 3 years ago

Ah yes @xperiandri you are right, it should be either of them. A switch/option however is still needed for this feature

Zaid-Ajaj commented 3 years ago

Hi @anthony-mi I have just reviewed the code and it looks really good! Thanks for the addition.

One thing you can simplify is to make createProjectFile of type bool instead of bool option with a default value of true:

let createProjectFile =
    if isNull parsedJson.["createProjectFile"]
    then true
    else parsedJson.["createProjectFile"].ToObject<bool>()

this makes the pieces of code that use the property somewhat simpler and removed duplicated branches. for example:

let generator =
  match config.createProjectFile with
  | None -> CodeGen.generateProjectDocument
  | Some true -> CodeGen.generateProjectDocument
  | Some false -> CodeGen.generatePropsDocument

would become

let generator =
  if config.createProjectFile 
  then CodeGen.generateProjectDocument
  else CodeGen.generateProjectDocument

Another thing you can do is pick a default value of createProjectFile before using it

let createProjectFile = defaultArg config.createProjectFile true

in case you think that bool option is a better type for the config but I would prefer the former approach.

What do you think?

I also don't mind merging right away and refactoring later on 😉

anthony-mi commented 3 years ago

in case you think that bool option is a better type for the config but I would prefer the former approach.

What do you think?

I also think that using the option is redundant here. Without it, the code will be simpler. I'll redo it.

Zaid-Ajaj commented 3 years ago

Merged and published as of Snowflaqe v1.23 🚀