{"action":"created","issue":{"url":"https://api.github.com/repos/fantix/gino/issues/198","repository_url":"https://api.github.com/repos/fantix/gino","labels_url":"https://api.github.com/repos/fantix/gino/issues/198/labels{/name}","comments_url":"https://api.github.com/repos/fantix/gino/issues/198/comments","events_url":"https://api.github.com/repos/fantix/gino/issues/198/events","html_url":"https://github.com/fantix/gino/pull/198","id":312534299,"number":198,"title":"Add support for inline model constraints","user":{"login":"kinware","id":2056837,"avatar_url":"https://avatars3.githubusercontent.com/u/2056837?v=4","gravatar_id":"","url":"https://api.github.com/users/kinware","html_url":"https://github.com/kinware","followers_url":"https://api.github.com/users/kinware/followers","following_url":"https://api.github.com/users/kinware/following{/other_user}","gists_url":"https://api.github.com/users/kinware/gists{/gist_id}","starred_url":"https://api.github.com/users/kinware/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kinware/subscriptions","organizations_url":"https://api.github.com/users/kinware/orgs","repos_url":"https://api.github.com/users/kinware/repos","events_url":"https://api.github.com/users/kinware/events{/privacy}","received_events_url":"https://api.github.com/users/kinware/received_events","type":"User","site_admin":false},"labels":[{"id":651779918,"url":"https://api.github.com/repos/fantix/gino/labels/help%20wanted","name":"help wanted","color":"128A0C","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":17,"created_at":"2018-04-09T13:38:10Z","updated_at":"2018-04-20T01:06:26Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/fantix/gino/pulls/198","html_url":"https://github.com/fantix/gino/pull/198","diff_url":"https://github.com/fantix/gino/pull/198.diff","patch_url":"https://github.com/fantix/gino/pull/198.patch"},"body":"Currently there are two ways to specify constraints and indexes covering multiple columns.\r\n\r\n1. Define the constraint or index outside the model class\r\n2. Define the constraint or index in the model class using the ```__table_args__``` attribute\r\n\r\nThe first option is (IMO) not as clean as keeping all details about the model inside the class. The second option works, but looks a bit ugly (again, IMO).\r\n\r\nThis PR adds a third alternative for specifying constraints and indexes \"inline\" (as sqlalchemy docs call it).\r\n\r\nThere are no tests yet (wanted to see if the feature is appropriate for inclusion or not first), but I guess adding something in test_declarative.py would be appropriate. Maybe define a model like the one in the docs in this PR, and then verify that the constraint is enforced (i.e insert same value twice).\r\n\r\nThoughts?"},"comment":{"url":"https://api.github.com/repos/fantix/gino/issues/comments/382930619","html_url":"https://github.com/fantix/gino/pull/198#issuecomment-382930619","issue_url":"https://api.github.com/repos/fantix/gino/issues/198","id":382930619,"user":{"login":"wwwjfy","id":126527,"avatar_url":"https://avatars0.githubusercontent.com/u/126527?v=4","gravatar_id":"","url":"https://api.github.com/users/wwwjfy","html_url":"https://github.com/wwwjfy","followers_url":"https://api.github.com/users/wwwjfy/followers","following_url":"https://api.github.com/users/wwwjfy/following{/other_user}","gists_url":"https://api.github.com/users/wwwjfy/gists{/gist_id}","starred_url":"https://api.github.com/users/wwwjfy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wwwjfy/subscriptions","organizations_url":"https://api.github.com/users/wwwjfy/orgs","repos_url":"https://api.github.com/users/wwwjfy/repos","events_url":"https://api.github.com/users/wwwjfy/events{/privacy}","received_events_url":"https://api.github.com/users/wwwjfy/received_events","type":"User","site_admin":false},"created_at":"2018-04-20T01:06:26Z","updated_at":"2018-04-20T01:06:26Z","author_association":"COLLABORATOR","body":"> This PR does not introduce the subclassing limitation, it already exists if you specify the constraints/indexes using the currently available method of using __table_args__. It does document the limitation though to at least highlight the potential issues to developers.\r\n\r\nYou're right, subclassing limitation is not introduced by this feature, though it applies here. What I meant is the limitation in `__table_args__` is expected, well known or easily found out in places like stack overflow, but for a new feature, I don't expect users to read documents carefully, so without proper error messages, it's not friendly enough.\r\n\r\n> Adding more meaningful error messages at runtime might actually be nice but I would suggest that is done in a separate PR to avoid cluttering this one. The \"inline constraint/index feature\" itself is 6 lines of code, but adding docs + tests makes this little PR close to 100 lines already :)\r\n\r\nCould you help to see if it's difficult or needs a huge chunk of code to give error messages? If not, I think this feature is more complete with that. If it's no easy task, I'd expect to see what errors it'll throw in the document, so that a user can easily find the cause and the solution. (Usually subclassing won't be the first guess if the user is not an expert on sqlalchemy, like me :) )"}}
{"actor":{"display_login":"wwwjfy"}}