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

def undiscard doesn't follow return false pattern #95

Closed
chiperific opened this issue Aug 15, 2023 · 1 comment · Fixed by #96
Closed

def undiscard doesn't follow return false pattern #95

chiperific opened this issue Aug 15, 2023 · 1 comment · Fixed by #96

Comments

@chiperific
Copy link
Contributor

chiperific commented Aug 15, 2023

Describe the bug
https://github.com/jhawthorn/discard/blob/master/lib/discard/model.rb#L141

def undiscard seems to violate its expected return value constraints.

    # Discard the record in the database
    #
    # @return [Boolean] true if successful, otherwise false
    def discard
      return false if discarded?
      run_callbacks(:discard) do
        update_attribute(self.class.discard_column, Time.current)
      end
    end

    ...

    # Undiscard the record in the database
    #
    # @return [Boolean] true if successful, otherwise false
    def undiscard
      return unless discarded? # <-------- returns nil
      run_callbacks(:undiscard) do
        update_attribute(self.class.discard_column, nil)
      end
    end

Shouldn't this be return false unless discarded??

Current behavior

> record = SomeModel.first
> record.discarded?
=> false
> record.undiscard # should return false
nil

Expected behavior

> record = SomeModel.first
> record.discarded?
=> false
> record.undiscard # should return false
false

Additional context

  • Ruby 3.1.4
  • Rails 7.0.7
@jarednorman
Copy link
Collaborator

Good find! I would accept a change that fixed this.

jarednorman added a commit that referenced this issue Aug 17, 2023
#95 Fix `undiscard` returns false unless discarded?
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 a pull request may close this issue.

2 participants