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

Adding ** to call. #48

Merged
merged 3 commits into from
Mar 16, 2021
Merged

Adding ** to call. #48

merged 3 commits into from
Mar 16, 2021

Conversation

adrianmann
Copy link

Please review this code to address the following issue:

#46

I ran my test suite against these changes and I no longer see the warnings. I also ran the kt-paperclip tests an they are passing.

@@ -47,7 +47,7 @@ def validate_blacklist(record, attribute, value)
end

def mark_invalid(record, attribute, types)
record.errors.add attribute, :invalid, options.merge(types: types.join(", "))
record.errors.add attribute, :invalid, **options.merge(types: types.join(", "))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [87/80]
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -4,7 +4,7 @@ module Paperclip
module Validators
class AttachmentPresenceValidator < ActiveModel::EachValidator
def validate_each(record, attribute, _value)
record.errors.add(attribute, :blank, options) if record.send("#{attribute}_file_name").blank?
record.errors.add(attribute, :blank, **options) if record.send("#{attribute}_file_name").blank?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [103/80]

@@ -39,7 +39,7 @@ def validate_each(record, attr_name, value)
unless value.send(CHECKS[option], option_value)
error_message_key = options[:in] ? :in_between : option
error_attrs.each do |error_attr_name|
record.errors.add(error_attr_name, error_message_key, filtered_options(value).merge(
record.errors.add(error_attr_name, error_message_key, **filtered_options(value).merge(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [102/80]

@adrianmann
Copy link
Author

The last build failed but is not failing locally for me. Could this be a random failure? I can't restart the build if so.

@ssinghi
Copy link

ssinghi commented Feb 5, 2021

@adrianmann Thanks for your PR. Can you plz remove the NEWS commit? The file isn't being maintained. This will trigger another build as well, and we will know if it was just a random fragile test failure. Thanks!

@adrianmann
Copy link
Author

I have removed the change to the NEWS file in the latest commit and tests passed this time, please review again.

@adrianmann
Copy link
Author

Bump! :)

@ssinghi ssinghi merged commit ab1016d into kreeti:master Mar 16, 2021
@adrianmann
Copy link
Author

adrianmann commented Mar 19, 2021

Thanks for merging this in, it seems the last build on master has failed. Can the build be restarted, I'd imagine it is an intermittent failure?

Edit: Also, when do you think the next release will be?

@Kevinrob
Copy link

@ssinghi Is it possible to have a new release with this code please?

@rstueven
Copy link

rstueven commented Sep 13, 2022

Is this fix included in 6.4.1? I'm still getting the wrong number of arguments (given 3, expected 1..2) error from Paperclip.io_adapters.for(uri_string, Paperclip::Attachment.default_options[:adapter_options])

Rails 7.0.3
Ruby 3.1.2
kt-paperclip 6.4.1

@adrianmann
Copy link
Author

@rstueven It is in 7.0.0. #46 (comment)

@rstueven
Copy link

@adrianmann Thanks!

Apparently my project uses the defunct paperclip-av-transcoder gem, and it relies on kt-paperclip ~> 6.4, so I'll have to figure out a workaround.

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.

4 participants