Support automatically generating `Identifiable` conformance on entities by LePips · Pull Request #61 · CreateAPI/CreateAPI

LePips · Jul 27, 2022

As getting comfortable with the project, generate Identifiable if available on the entity.

I initially had Hashable as well however that should be behind some config option because only structs have automatic conformance. I think all built in types can be statically listed and that hashable list is of course a subset of that list. Since Hashable would be desired in both makeOneOf and 'makeObject', for the future:

extension Array where Element == Property {
    var allHashable: Bool {
        let hashable = // list of all hashable types
        return entity.properties.allSatisfy { hashable.contains($0.type.builtinTypeName ?? "") }
    }
}

If this needs to be behind a config flag, I'm okay to do that.

liamnichols

Awesome! Thanks for the contribution @LePips!

Identifiable is only available on iOS 13+ and while we don't officially state which OS version we try and maintain compatibility with, I know that some people use CreateAPI while still supporting iOS 12 (me 😬).

I think putting this behind a flag is a good approach! Are you able to give that a go?

Secondly, we'll want to make sure that this change is covered in some of our test fixtures so that nobody mistakenly breaks your change in the future 🙏 This process isn't very well documented at the moment, but either adapting an existing test or adding a new one to GenerateOptionsTests.swift should do the trick.

You'll need to temporarily update the projectPath in Snapshots.swift and add a GENERATE_SNAPSHOTS environment variable that is set to true for it to update the expected fixtures. Let me know if you get stuck, I know its not the smoothest process at the moment 👍

Sources/CreateAPI/Generator/Generator+Schemas.swift

LePips · Jul 27, 2022

Oops, didn't think about the iOS versions. Thanks for the comment! I'll get on the tests.

liamnichols · Jul 27, 2022

Just one more thought, if somebody used the rename options in their config:

rename:
  properties:
    User.user_id: id
    identifier: id

Would we be able to pick this up and annotate User as well as any other entity with the property identifier in their schema as Identifiable?

I ask because I am not too familiar where the renaming takes place so we might need to adjust how we check?

LePips · Jul 27, 2022

It will indeed attach Identifiable in that case with the replacement. The replacement of properties happen in L720 in Generator+Schemas, which happen when the entity properties are being set right before the protocols.

liamnichols

Looking good!

I enabled the CI checks on this PR but they failed since we also need to commit the package that is generated by the tests.

You can do that by setting the environment variable and running the tests locally:

README.md

Tests/CreateAPITests/GenerateOptionsTests.swift

LePips · Jul 27, 2022

Thanks for showing me the ropes!

Sources/CreateAPI/Generator/Generator+Schemas.swift

Co-authored-by: Liam Nichols <liam.nichols.ln@gmail.com>

LePips · Jul 27, 2022

Sources/CreateAPI/Generator/Generator+Schemas.swift

    }

-    private func getProtocols(for type: TypeName, context: Context) -> Protocols {
+    private func getProtocols(for entity: EntityDeclaration, context: Context) -> Protocols {


I just went ahead and will pass in the entire entity, mainly because I think we will have to check the nested types for Hashable declaration. I will take a look at that next and if not, I will change to just passing the properties there.

LePips · Jul 27, 2022

Sources/CreateAPI/Generator/Generator+Schemas.swift

        entity.protocols = {
-            var protocols = getProtocols(for: name, context: context)
+            var protocols = getProtocols(for: entity, context: context)
            let hashable = Set(["String", "Bool", "URL", "Int", "Double"]) // TODO: Add support for more types


These will be removed when getting to Hashable

liamnichols

Good job! Looking great! Just one more thing to get the tests passing again and then I think that its good to go 🚀

Tests/CreateAPITests/Specs/petstore.yaml

LePips · Jul 27, 2022

Sorry this PR has taken a bit of back and forth, I'm really appreciative of it and showing me how things should be done around here. I'll get the hang of it soon. This project is great! We're going to be moving from OpenAPI after a few things are done.

liamnichols

Sorry this PR has taken a bit of back and forth, I'm really appreciative of it and showing me how things should be done around here. I'll get the hang of it soon. This project is great! We're going to be moving from OpenAPI after a few things are done.

No, thank you for taking the time to work on this! Your contribution is very valuable and it's good that we get to work though this 🙇

I think we just need to revert that change still (reasoning inline) but then this is good to go 🎉

Thanks again!

Tests/CreateAPITests/Specs/petstore.yaml

Tests/CreateAPITests/Expected/petstore-no-package/Entities.swift

This reverts commit 40c340e.

liamnichols

Great work on this! Everything looks good to me 🚀

I might work on the documentation improvements over the weekend and then get a release out after that. But if you want this in a tagged release before then, let me know!

generate identifiable if available

da0e0b9

liamnichols reviewed Jul 27, 2022

View reviewed changes

Sources/CreateAPI/Generator/Generator+Schemas.swift Outdated Show resolved Hide resolved

LePips added 3 commits July 27, 2022 11:25

add config option and check for id properly hashable

eab8b88

Update README.md

864184d

add identifiable tests

3a051b6

LePips requested a review from liamnichols July 27, 2022 17:44

liamnichols reviewed Jul 27, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Tests/CreateAPITests/GenerateOptionsTests.swift Outdated Show resolved Hide resolved

Tests/CreateAPITests/GenerateOptionsTests.swift Outdated Show resolved Hide resolved

remove default test, is default

a6d2e18

liamnichols reviewed Jul 27, 2022

View reviewed changes

Tests/CreateAPITests/GenerateOptionsTests.swift Outdated Show resolved Hide resolved

LePips added 2 commits July 27, 2022 12:53

change config name and fix tests

fd03230

test inclusion for renaming

edefeb1

LePips requested a review from liamnichols July 27, 2022 18:54

liamnichols changed the title ~~Generate Identifiable if Available~~ Support automatically generating Identifiable conformance on entities Jul 27, 2022

liamnichols reviewed Jul 27, 2022

View reviewed changes

Sources/CreateAPI/Generator/Generator+Schemas.swift Outdated Show resolved Hide resolved

Sources/CreateAPI/Generator/Generator+Schemas.swift Outdated Show resolved Hide resolved

LePips and others added 2 commits July 27, 2022 13:33

Update Sources/CreateAPI/Generator/Generator+Schemas.swift

4f32e80

Co-authored-by: Liam Nichols <liam.nichols.ln@gmail.com>

fix for inlinable allof references

a4a5d32

LePips commented Jul 27, 2022

View reviewed changes

liamnichols reviewed Jul 27, 2022

View reviewed changes

Tests/CreateAPITests/Specs/petstore.yaml Outdated Show resolved Hide resolved

update tests from schema

40c340e

liamnichols reviewed Jul 28, 2022

View reviewed changes

Tests/CreateAPITests/Specs/petstore.yaml Outdated Show resolved Hide resolved

Tests/CreateAPITests/Expected/petstore-no-package/Entities.swift Outdated Show resolved Hide resolved

liamnichols mentioned this pull request Jul 28, 2022

Consider making isInliningPropertiesFromReferencedSchemas the default behaviour #63

Closed

LePips added 2 commits July 28, 2022 09:25

Revert "update tests from schema"

b1efe31

This reverts commit 40c340e.

revert allOf testing

a270c67

LePips requested a review from liamnichols July 28, 2022 15:29

liamnichols approved these changes Jul 28, 2022

View reviewed changes

liamnichols merged commit 357c187 into CreateAPI:main Jul 28, 2022

Search code, repositories, users, issues, pull requests...

Comments

Conversation

LePips commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamnichols left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LePips commented Jul 27, 2022

Uh oh!

liamnichols commented Jul 27, 2022

Uh oh!

LePips commented Jul 27, 2022

Uh oh!

liamnichols left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LePips commented Jul 27, 2022

Uh oh!

Uh oh!

Uh oh!

LePips Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

LePips Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

liamnichols left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LePips commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamnichols left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liamnichols left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LePips commented Jul 27, 2022 •

edited

Loading

LePips commented Jul 27, 2022 •

edited

Loading