Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nullable and crate_index in ORM column definition #481

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

hammerhead
Copy link
Member

Summary of the changes / Why this is an improvement

As per the title.

Checklist

  • CLA is signed

'tags ARRAY(OBJECT), \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_table_with_nullable(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_with_pk_nullable(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
self.Base.metadata.create_all()

def test_table_without_index(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_table_without_index_unsupported(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
@hammerhead hammerhead marked this pull request as ready for review December 8, 2022 11:03
@hammerhead hammerhead requested a review from amotl December 8, 2022 11:04
Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this excellent improvement. I've added some suggestions, mostly about wording and naming things.

CHANGES.txt Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/compiler.py Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved
src/crate/client/sqlalchemy/compiler.py Outdated Show resolved Hide resolved
Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

.

@hammerhead hammerhead requested a review from amotl December 8, 2022 14:15
Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

👍

@amotl
Copy link
Member

amotl commented Dec 8, 2022

Please note that the notices from CodeQL currently do not collapse, even when explicitly dismissed. For more background on this, see github/codeql-action#1411.

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.

2 participants