Skip to content

Support length for various fields#176

Merged
dseomn merged 11 commits into
google:mainfrom
hazyd:length
Oct 3, 2022
Merged

Support length for various fields#176
dseomn merged 11 commits into
google:mainfrom
hazyd:length

Conversation

@hazyd
Copy link
Copy Markdown

@hazyd hazyd commented Sep 13, 2022

No description provided.

Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
if self._nullable:
return self._type.ddl()
return '{field_type} NOT NULL'.format(field_type=self._type.ddl())
return f'{base_ddl}'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return base_ddl

Comment thread spanner_orm/field.py Outdated
@dgorelik dgorelik requested a review from dseomn September 15, 2022 17:45
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/tests/models.py Outdated
Comment thread spanner_orm/tests/models.py Outdated
Copy link
Copy Markdown
Contributor

@dseomn dseomn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got more nits on the code, but I still want to figure out the API more, so I'm not sure it's worth addressing the nits until we have a better idea of what the API should look like. (See the comment from my previous review.)

Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Copy link
Copy Markdown
Contributor

@dseomn dseomn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
@hazyd
Copy link
Copy Markdown
Author

hazyd commented Sep 29, 2022

Ready for another round of review now.

Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/tests/field_test.py
Comment thread spanner_orm/tests/field_test.py
Comment thread spanner_orm/field.py Outdated
Comment thread spanner_orm/tests/field_test.py
Copy link
Copy Markdown
Contributor

@dseomn dseomn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm not sure why status checks aren't running.

@dseomn
Copy link
Copy Markdown
Contributor

dseomn commented Oct 3, 2022

Mind pulling in the latest changes from #190 ? I'm hoping that will get github to run the tests on this so we can merge it.

@hazyd
Copy link
Copy Markdown
Author

hazyd commented Oct 3, 2022

Just rebased. It seems that it requires you to approve running workflows.

@dseomn dseomn merged commit e2f103b into google:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants