Skip to content

Conversation

alexei
Copy link
Contributor

@alexei alexei commented Apr 14, 2022

https://code.djangoproject.com/ticket/33607

The TL;DR is PostgresIndex.create_sql accepts a using argument but throws it away. This update fixes that and passes the argument to the underlying method.

@alexei
Copy link
Contributor Author

alexei commented Apr 14, 2022

I noticed subclasses of IndexTestMixin declare an index_class attribute but don't make use of it. Is this chance or design? I thought it was the former and wanted to clean them up, but am unsure about the policy here.

@felixxm
Copy link
Member

felixxm commented Apr 14, 2022

I noticed subclasses of IndexTestMixin declare an index_class attribute but don't make use of it. Is this chance or design? I thought it was the former and wanted to clean them up, but am unsure about the policy here.

index_class is used in tests inherited from IndexTestMixin.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@alexei Thanks 👍 I left comments.

@alexei
Copy link
Contributor Author

alexei commented Apr 15, 2022

I noticed subclasses of IndexTestMixin declare an index_class attribute but don't make use of it. Is this chance or design? I thought it was the former and wanted to clean them up, but am unsure about the policy here.

index_class is used in tests inherited from IndexTestMixin.

That's what I'm saying: it's not. Only the IndexTestMixin methods use index_class, while the subclasses repeat the class.

@felixxm
Copy link
Member

felixxm commented Apr 15, 2022

I noticed subclasses of IndexTestMixin declare an index_class attribute but don't make use of it. Is this chance or design? I thought it was the former and wanted to clean them up, but am unsure about the policy here.

index_class is used in tests inherited from IndexTestMixin.

That's what I'm saying: it's not. Only the IndexTestMixin methods use index_class, while the subclasses repeat the class.

I don't anything wrong or unusual in this pattern 🤔

@alexei
Copy link
Contributor Author

alexei commented Apr 15, 2022

I don't anything wrong or unusual in this pattern 🤔

It breaks the open-closed principle. If e.g. GinIndexTests didn't hard code the index class, I could've extended GinIndex for the dummy index and GinIndexTests for its tests 🤷‍♂️

@felixxm
Copy link
Member

felixxm commented Apr 15, 2022

It breaks the open-closed principle. If e.g. GinIndexTests didn't hard code the index class, I could've extended GinIndex for the dummy index and GinIndexTests for its tests.

... but GinIndexTests was never intended to be extended. Extending GinIndexTests for DummyIndex would create even more unnecessary (and/or redundant) tests for DummyIndex that we don't need/want.

@felixxm felixxm changed the title Refs #33607 -- Update PostgresIndex.create_sql to pass using argument to schema editor Fixed #33607 -- Made PostgresIndex.create_sql() respect the "using" argument. Apr 15, 2022
@felixxm
Copy link
Member

felixxm commented Apr 15, 2022

@alexei Welcome aboard ⛵

@felixxm felixxm merged commit a1e4e86 into django:main Apr 15, 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.

2 participants