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

missing_presence_validation: ignore counter cache columns #180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

This check produces false positives for counter cache columns.
These are expected to be set by ActiveRecord, not by users.

@fatkodima
Copy link
Contributor Author

@gregnavis Can you please merge all the open PRs?

@gregnavis
Copy link
Owner

Thanks for the nudge, @fatkodima! I won't merge them all today, as I'd like to see some changes made, but I definitely need to pay attention to the outstanding PRs. I left some comments on a few other PRs.

Here, I'm not sure what to do with older Rails versions. I think we'll drop 4.2 soon (I'm sure you'd love that decision!), but that would require a new major release.

For now, I suggest the following: Rails versions that fail in CI due to outdated APIs should:

  1. Show a warning to the user with the following content: "missing_presence_validation: counter cache columns will receive complaints about missing presence validations in Rails versions older than X.Y; you can add them to ignore_attributes"
  2. Skip counter cache detection for those older Rails versions.

@fatkodima, does that make sense?

@fatkodima
Copy link
Contributor Author

Sorry, I am lazy and will wait when you drop the old version.

Imo, you can easily drop activerecord < 7 and release v2 version of the gem. People on rails 4.2 don't care about best practices, so they are not the target audience of this gem.

@gregnavis gregnavis added the false positive Pull requests that fix false positives label Aug 27, 2024
@gregnavis gregnavis added this to the 2.0 milestone Aug 27, 2024
@gregnavis
Copy link
Owner

@fatkodima, you are not lazy at all! I totally understand supporting 4.2 is a total pain in the ass.

I marked this and a few other PRs for 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false positive Pull requests that fix false positives
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants