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

Discrete cleanup #1401

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Discrete cleanup #1401

merged 3 commits into from
Jul 8, 2024

Conversation

Earlopain
Copy link
Collaborator

@Earlopain Earlopain commented Jul 8, 2024

  • v4_ready? can always return true on v4. Deprecation?
  • Nothing checks against discrete_support?/is_discrete? anymore
  • Benchmark to compare the two doesn't do anything anymore
  • Remove some duplicated tests now that they both do the same
  • Ignore the is_discrete. I don't believe this can safely be dropped beforehand. Probably a v5 thing.

@bensheldon
Copy link
Owner

Thanks for doing this! This is great!

v4_ready? can always return true on v4. Deprecation?

Yes, I think returning true on v4, and adding a deprecation warning to get people to remove it would be good.

If you're reading this deprecation you're already all good (:
@Earlopain
Copy link
Collaborator Author

Alright, added a deprecation for that.

Should I skip ignored_columns for is_discrete for now? I saw you added that as a task for 4.99 in #1407.

@bensheldon
Copy link
Owner

This looks really good. Thank you for this!

Should I skip ignored_columns for is_discrete for now?

I think let's leave it.

I am a little reluctant to have ignored_columns because it changes the SQL queries from a SELECT * to explicit columns (but maybe that's good? but still different), but I don't see a safe way to only add it solely in a x.99 transitional release (because that only allows for a code change, or a migration change, but not both at the same time).

@bensheldon bensheldon added the refactor Code changes that do not introduce new features label Jul 8, 2024
@bensheldon bensheldon merged commit df41bce into bensheldon:main Jul 8, 2024
14 checks passed
@Earlopain
Copy link
Collaborator Author

Yeah, I can see where you're coming from with that. All the additional features introduced previously were nicely guarded behind column checks and whatnot which should be sufficient.

Crossing my finders that you won't have to come back to this one for quite a while yet

@Earlopain Earlopain deleted the discrete-cleanup branch July 8, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code changes that do not introduce new features
Projects
Development

Successfully merging this pull request may close these issues.

2 participants