Kitura / Swift-Kuery-ORM

An ORM for Swift, built on Codable
Apache License 2.0
212 stars 30 forks source link

How about changing the variable name `idKeypath`? #122

Open fummicc1 opened 5 years ago

fummicc1 commented 5 years ago

Suggestion. How about changing the variable name idKeypath?

Nice to meet you, I recently started studying Kitura. Thank you for your daily maintenance! This is my personal opinion, but I would be happy if you could refer to it.

This is a quote from this site,

Using optional ID properties Declaring your ID property as optional allows the ORM to assign the ID automatically when the model is saved. If the value of ID is nil, the database will assign an auto-incremented value. At present this is only support for an Int? type.

You may instead provide an explicit value, which will be used instead of automatic assignment.

Optional IDs must be identified by defining the idKeypath: IDKeyPath property, as in the example below:


struct Person: Model {
var id: Int?
var firstname: String
var surname: String
var age: Int
static var idKeypath: IDKeyPath = \Person.id

}


I think the `idKeypath` property should be written in LowerCamelCase. What do you think?
In fact, I didn't work because I declared the `idKeyPath` property.

### Environment Details
- MacOS  ... Catalina 10.15 beta
- Xcode ... 10.3
- Swift ... 5.0

### Steps to Reproduce
1) ... 
Define a structure that inherits Model, declare `idKeyPath` as a property, and confirm that it does not work.
2) ... 
You will notice that it is an `idKeypath` property instead of an `idKeyPath` property, and fix the code. I think it works well.

### Expected vs. Actual Behaviour
- [ ] Expected to happen (I would like to write like this) ...
```swift
struct Diary: Equatable, Model {
    var id: Int?
    var title: String?

    static var idKeyPath: IDKeyPath = \Diary.id
}
djones6 commented 5 years ago

I agree that it would have been better to have an uppercase P, however this would be a breaking change. I'm not sure that we would want to break the API for the sake of correcting casing.

However, it might be possible to add an additional property idKeyPath that essentially acts as an alias of idKeypath. @kilnerm do you think this is feasible?

fummicc1 commented 5 years ago

@djones6 Thankyou for your reply. idKeypath is not bad at all. But as for me, idKeyPath is suitable because Swift's property is lowerCamelCase style! Though I will comment #123 soon, #123 PR may be non-breaking. Please look at below Image.

Note: If my PR is passed, a part of README.md where idKeypath is referred to should be change.

スクリーンショット 2019-09-24 19 37 33