fix: Allow adding of new schema to database #635
Conversation
AndrewSisley
added
bug
Codecov Report
@@ Coverage Diff @@
## develop #635 +/- ##
===========================================
+ Coverage 57.00% 57.09% +0.09%
===========================================
Files 122 122
Lines 14636 14638 +2
===========================================
+ Hits 8343 8358 +15
+ Misses 5578 5566 -12
+ Partials 715 714 -1
|
| @@ -1266,6 +1261,16 @@ func (g *Generator) genTypeQueryableFieldList( | |||
| return field | |||
| } | |||
|
|
|||
| func (g *Generator) appendIfNotExists(obj gql.Type) error { | |||
There was a problem hiding this comment.
suggestion(non-blocking): I read this as some element named Not that exists. Perhaps a rename to appendIfNewSchema or appendIfNew ?
There was a problem hiding this comment.
Definitely not a fan of appendIfNewSchema, as that is incorrect/overly-specific. I'm pretty sure I have seen IfNotExists in a number of code bases before, I don't think it is particularly uncommon. appendIfNew might be seen as slightly misleading in another direction, suggesting that new instances (of the same name) will still be appended, but THB I don't think appendIfNotExists vs appendIfNew matters at all.
There was a problem hiding this comment.
ifNotExist is quite fine in my opinion. I would remove the last s though.
There was a problem hiding this comment.
😆 appendIfNotExists I think fits with what I have seen before, but running it over in my head I can't quite figure out whether with the trailing s is more or less correct than the current - I think without an s is logically more correct, but it feels slightly wrong lol @orpheuslummis ? 🤣
There was a problem hiding this comment.
Go uses os.ErrNotExist 🙂
There was a problem hiding this comment.
Keeping it with the s as I see this as a singular 3rd person verb lol
There was a problem hiding this comment.
Keeping it with the s as I see this as a singular 3rd person verb lol
feels like there is another grammar rule at play here though RE negatives - it exists vs it does not exist - maybe in English the negative is forced to use the infinitive? Feels weird that I dont know this stuff lol
There was a problem hiding this comment.
I like to follow what the language uses: https://pkg.go.dev/os#IsExist
There was a problem hiding this comment.
That's no fun though :) And it feels like that lib is breaking English, assuming exist is not an adjective, then IsExist should really be DoesExist (not great programming-wise as Is is a nice prefix for bools), or IsExisting?
Wish I knew this stuff better lol
There was a problem hiding this comment.
The function I linked to is returning a bool so IsExist makes sense according to your logic. They also use it for errors: https://pkg.go.dev/os#pkg-variables
Test failed before the fix went in - each item in that testCase slice corresponds to a |
Ok thanks for clarifying :) |
Should be using the schema type map, is cleaner, and correct if adding a new schema to existing set.
AndrewSisley
force-pushed
the
sisley/fix/I480-second-schema
branch
from
79ac586
to
ae20a41
Compare
* Allow adding of new schema * Remove extra variable Should be using the schema type map, is cleaner, and correct if adding a new schema to existing set.
Relevant issue(s)
Resolves #489
Description
Allows calling
db.AddSchemamultiple times - this was broken at some point whilst implementing aggregates - the generator code would try and add existing types a second time which would error. This checks checks whether the aggregate input types already exist before trying to add them.