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

Move without_versioning to model #1064

Conversation

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented Mar 15, 2018

The current implementation ofwithout_versioning is incorrectly set as an instance method when it should be part of the class which the current behaviour clearly shows. This PR fixes this and deprecates the old behavior.

The only difference with the new method is that it no longer accepts a method name, as its on the class not the instance. I didnt want to add this to the new implementation because I believe it is an unnecessary addition to the API.

This was somewhat related to Issue #916

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

The current implementation of without_versioning is incorrectly set as an instance method when it should be part of the class ..

I agree it's misleading. However, instead of moving it, we could just drop it and people could use PaperTrail.request(enabled_for_controller: false) do .., right?

Please rebase.

@westonganger westonganger force-pushed the westonganger:move_without_versioning_to_model branch from 053d953 to 9d5a690 Mar 17, 2018

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

Rebased.

Your suggestion could work but enabled_for_controller seriously needs to be renamed its awful, especially for this use case. Its not even a good name for the controller method because it says enabled for controller but really its enabled for the request/action.

Why not change it to without_versioning. Ex.

PaperTrail.request(without_versioning: true) do .. end

and the controller method name

class MyController < ApplicationController

  def paper_trail_request_without_versioning
  end

end
@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

Another note is that by removing without_versioning for the model and only having PaperTrail.request() do .. end , there would no longer be an option to disable only 1 model as its now disabled for all models within that block. But I'm not sure that even matters because anyone should be able to easily organize the code around this.

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

Closing via #1069, thanks Weston for talking it through with me.

@jaredbeck jaredbeck closed this Mar 24, 2018

@westonganger westonganger deleted the westonganger:move_without_versioning_to_model branch May 18, 2018

@westonganger westonganger restored the westonganger:move_without_versioning_to_model branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.