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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce repeated/unnecessary code #354

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

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Jun 16, 2017

just wanted to make a tiny change ... then things escalated 馃槶
@danielmorrison @domcleal

  • make audited_attributes and audited_changes follow the same logic regarding only & except and privacy level
  • remove runtime .to_s-ing of only & except
  • remove duplicate non_audited_columns instance method
  • remove non_audited_columns class method in favor of either calling audited except: [] or modifying audited_options
  • remove unnecessary .flatten
  • test actual auditing behavior instead of testing class accessors
  • test actual recorded columns instead of testing for if they include certain things to make tests catch empty hashes / wrong keys

warning: records_changes_to_specified_fields? seems to be untested ... but that's for another PR

@grosser
Copy link
Contributor Author

grosser commented Jun 16, 2017

... I'd love to take this even further and make create + destroy also simply audit changes ... that would unify the stored data and remove redundant fields in create/destroy audits ...

@danielmorrison
Copy link
Member

Looks good to me after a quick glance. Should we add any deprecation methods/warnings?

@grosser
Copy link
Contributor Author

grosser commented Jun 16, 2017

warnings just get ignores ... if someone updates this will blow up and they have to find out what to do ... could keep the method defined and raise a NotImplementedError instead though ?

@grosser
Copy link
Contributor Author

grosser commented Jun 18, 2017

added NotImplementedError for removed methods ... good to go ?

@grosser
Copy link
Contributor Author

grosser commented Jun 19, 2017

@danielmorrison

@tbrisker
Copy link
Collaborator

@grosser I think a lot of the complexity and duplication has been resolved in various other PRs that were recently merged. Are there some changes that are still required? If so, could you please update your PR?

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.

None yet

3 participants