Skip to content

Commit

Permalink
Make some api-private methods actually private
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaredbeck
jaredbeck committed Nov 26, 2022
1 parent 9a48faf commit 71f5212
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 79 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).
`PaperTrail::RecordTrail#update_columns`
- [#1404](https://github.com/paper-trail-gem/paper_trail/pull/1404) -
Delay referencing ActiveRecord until after Railtie is loaded
- Where possible, methods which are not part of PaperTrail's public API have
had their access changed to private. All of these methods had been clearly
marked as `@api private` in the documentation, for years. This is not expected
to be a breaking change.

## 13.0.0 (2022-08-15)

Expand Down
3 changes: 2 additions & 1 deletion lib/paper_trail.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,15 @@ def serializer

# Returns PaperTrail's global configuration object, a singleton. These
# settings affect all threads.
# @api private
# @api public
def config
@config ||= PaperTrail::Config.instance
yield @config if block_given?
@config
end
alias configure config

# @api public
def version
VERSION::STRING
end
Expand Down
112 changes: 56 additions & 56 deletions lib/paper_trail/record_trail.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,6 @@ def clear_version_instance
@record.send("#{@record.class.version_association_name}=", nil)
end

# Is PT enabled for this particular record?
# @api private
def enabled?
PaperTrail.enabled? &&
PaperTrail.request.enabled? &&
PaperTrail.request.enabled_for_model?(@record.class)
end

# Returns true if this instance is the current, live one;
# returns false if this instance came from a previous version.
def live?
Expand Down Expand Up @@ -75,13 +67,6 @@ def record_create
end
end

# PT-AT extends this method to add its transaction id.
#
# @api private
def data_for_create
{}
end

# `recording_order` is "after" or "before". See ModelConfig#on_destroy.
#
# @api private
Expand All @@ -105,13 +90,6 @@ def record_destroy(recording_order)
end
end

# PT-AT extends this method to add its transaction id.
#
# @api private
def data_for_destroy
{}
end

# @api private
# @param force [boolean] Insert a `Version` even if `@record` has not
# `changed_notably?`.
Expand Down Expand Up @@ -141,40 +119,6 @@ def record_update(force:, in_after_callback:, is_touch:)
end
end

# PT-AT extends this method to add its transaction id.
#
# @api private
def data_for_update
{}
end

# @api private
# @return - The created version object, so that plugins can use it, e.g.
# paper_trail-association_tracking
def record_update_columns(changes)
return unless enabled?
data = Events::Update.new(@record, false, false, changes).data

# Merge data from `Event` with data from PT-AT. We no longer use
# `data_for_update_columns` but PT-AT still does.
data.merge!(data_for_update_columns)

versions_assoc = @record.send(@record.class.versions_association_name)
version = versions_assoc.create(data)
if version.errors.any?
log_version_errors(version, :update)
else
version
end
end

# PT-AT extends this method to add its transaction id.
#
# @api private
def data_for_update_columns
{}
end

# Invoked via callback when a user attempts to persist a reified
# `Version`.
def reset_timestamp_attrs_for_update_if_needed
Expand Down Expand Up @@ -298,13 +242,69 @@ def build_version_on_update(force:, in_after_callback:, is_touch:)
@record.class.paper_trail.version_class.new(data)
end

# PT-AT extends this method to add its transaction id.
#
# @api public
def data_for_create
{}
end

# PT-AT extends this method to add its transaction id.
#
# @api public
def data_for_destroy
{}
end

# PT-AT extends this method to add its transaction id.
#
# @api public
def data_for_update
{}
end

# PT-AT extends this method to add its transaction id.
#
# @api public
def data_for_update_columns
{}
end

# Is PT enabled for this particular record?
# @api private
def enabled?
PaperTrail.enabled? &&
PaperTrail.request.enabled? &&
PaperTrail.request.enabled_for_model?(@record.class)
end

def log_version_errors(version, action)
version.logger&.warn(
"Unable to create version for #{action} of #{@record.class.name}" \
"##{@record.id}: " + version.errors.full_messages.join(", ")
)
end

# @api private
# @return - The created version object, so that plugins can use it, e.g.
# paper_trail-association_tracking
def record_update_columns(changes)

This comment has been minimized.

Sign in to view

Copy link
@bf4

bf4 Nov 29, 2022

oh, I like this method :( (though I admit it said api private before)

we're using it to define a mixin version_update_columns which basically adds (opt-in) versioning to any update_columns calls

it's defined like

    # https://github.com/paper-trail-gem/paper_trail/blob/v12.2.0/lib/paper_trail/record_trail.rb#L216-L229
    def version_update_columns(**attributes_to_update)
      assign_attributes(**attributes_to_update)
      versioned_changes = {}
      attributes_to_update.each do |k, v|
        versioned_changes[k] = changes[k].presence || [self[k], v]
      end

      update_columns(**attributes_to_update)
      paper_trail.record_update_columns(versioned_changes)
      # paper_trail.update_columns(**attributes_to_update)
    end

would you be open to having our method or something like it added to the public api?
return unless enabled?
data = Events::Update.new(@record, false, false, changes).data

# Merge data from `Event` with data from PT-AT. We no longer use
# `data_for_update_columns` but PT-AT still does.
data.merge!(data_for_update_columns)

versions_assoc = @record.send(@record.class.versions_association_name)
version = versions_assoc.create(data)
if version.errors.any?
log_version_errors(version, :update)
else
version
end
end

def version
@record.public_send(@record.class.version_association_name)
end
Expand Down
44 changes: 22 additions & 22 deletions lib/paper_trail/request.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,28 +75,6 @@ def enabled_for_model?(model)
!!store.fetch(:"enabled_for_#{model}", true)
end

# @api private
def merge(options)
options.to_h.each do |k, v|
store[k] = v
end
end

# @api private
def set(options)
store.clear
merge(options)
end

# Returns a deep copy of the internal hash from our RequestStore. Keys are
# all symbols. Values are mostly primitives, but whodunnit can be a Proc.
# We cannot use Marshal.dump here because it doesn't support Proc. It is
# unclear exactly how `deep_dup` handles a Proc, but it doesn't complain.
# @api private
def to_h
store.deep_dup
end

# Temporarily set `options` and execute a block.
# @api private
def with(options)
Expand Down Expand Up @@ -133,6 +111,19 @@ def whodunnit

private

# @api private
def merge(options)
options.to_h.each do |k, v|
store[k] = v
end
end

# @api private
def set(options)
store.clear
merge(options)
end

# Returns a Hash, initializing with default values if necessary.
# @api private
def store
Expand All @@ -141,6 +132,15 @@ def store
}
end

# Returns a deep copy of the internal hash from our RequestStore. Keys are
# all symbols. Values are mostly primitives, but whodunnit can be a Proc.
# We cannot use Marshal.dump here because it doesn't support Proc. It is
# unclear exactly how `deep_dup` handles a Proc, but it doesn't complain.
# @api private
def to_h
store.deep_dup
end

# Provide a helpful error message if someone has a typo in one of their
# option keys. We don't validate option values here. That's traditionally
# been handled with casting (`to_s`, `!!`) in the accessor method.
Expand Down

0 comments on commit 71f5212

Please sign in to comment.