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

Fix index class argument name not work #1856

Merged
merged 6 commits into from
Jan 22, 2025

Conversation

waketzheng
Copy link
Contributor

@waketzheng waketzheng commented Jan 21, 2025

Description

Argument name of Index class does not pass to the _get_index_sql function when generating schemas.

Motivation and Context

  1. Pass index.name to _get_index_sql
  2. Add field_names property to Index class
  3. Add unittest for Index with name defined

How Has This Been Tested?

make ci

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@waketzheng waketzheng requested a review from henadzit January 21, 2025 08:14
Copy link

codspeed-hq bot commented Jan 21, 2025

CodSpeed Performance Report

Merging #1856 will not alter performance

Comparing waketzheng:refactor-generate-index-sql (cf39f47) with develop (de48e77)

Summary

✅ 8 untouched benchmarks

@coveralls
Copy link

coveralls commented Jan 21, 2025

Pull Request Test Coverage Report for Build 12900145277

Details

  • 15 of 21 (71.43%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 89.421%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tortoise/indexes.py 10 16 62.5%
Totals Coverage Status
Change from base Build 12889066835: -0.02%
Covered Lines: 6492
Relevant Lines: 7078

💛 - Coveralls

@waketzheng
Copy link
Contributor Author

@henadzit Shall we add index_name and get_sql back to Index class as follows for compatibility:

class Index:
    def index_name(self, schema_generator: "BaseSchemaGenerator", model: "Type[Model]") -> str:
        return self.name or schema_generator._generate_index_name("idx", model, self.field_names)

    def get_sql(self, schema_generator: "BaseSchemaGenerator", model: "Type[Model]", safe: bool) -> str:
        return schema_generator._get_index_sql(
            model, self.field_names, safe, index_name=self.name, index_type=self.INDEX_TYPE, extra=self.extra
        )

@henadzit
Copy link
Contributor

Thanks for fixing this!

@henadzit Shall we add index_name and get_sql back to Index class as follows for compatibility:

class Index:
    def index_name(self, schema_generator: "BaseSchemaGenerator", model: "Type[Model]") -> str:
        return self.name or schema_generator._generate_index_name("idx", model, self.field_names)

    def get_sql(self, schema_generator: "BaseSchemaGenerator", model: "Type[Model]", safe: bool) -> str:
        return schema_generator._get_index_sql(
            model, self.field_names, safe, index_name=self.name, index_type=self.INDEX_TYPE, extra=self.extra
        )

I searched the aerich code base and seems to be used there - this is something that I didn't realize. We probably need to get it back. Let me know if you want to do it yourself or you want me to do it. If you decide to do it yourself, please, leave a comment in the code to note that it is needed for aerich. Thanks!

henadzit
henadzit previously approved these changes Jan 21, 2025
@waketzheng
Copy link
Contributor Author

I searched the aerich code base and seems to be used there - this is something that I didn't realize. We probably need to get it back. Let me know if you want to do it yourself or you want me to do it. If you decide to do it yourself, please, leave a comment in the code to note that it is needed for aerich. Thanks!

@henadzit Cloud you review again. I added index_name/get_sql back and use index.get_sql in schema_generator.

Copy link
Contributor

@henadzit henadzit left a comment

Choose a reason for hiding this comment

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

LGTM!

@waketzheng waketzheng merged commit 948ccdb into tortoise:develop Jan 22, 2025
9 checks passed
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