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

Feature request: Don't set "local.path" as a default filename to prevent it from being detected as a binary #69

Open
ts-3156 opened this issue May 21, 2021 · 5 comments

Comments

@ts-3156
Copy link

ts-3156 commented May 21, 2021

Thank you for useful gem!

On some systems, a preview of an image is not displayed correctly when I specify "local.path" as a filename. To solve this behavior, could you simply set "file" as a default filename?

If you agree with this issue, I will create a pull request. I would like to know your opinion.


One of those systems is Slack for Mac. Starting at 2021/05/05 05:30 UTC, Slack client no longer correctly displays previews of images when the filename is "local.path".

Expectation Reality
Screenshot 2021-05-07 21 52 06 Screenshot 2021-05-07 21 47 43

The relevant code of this issue is here.
https://github.com/socketry/multipart-post/blob/master/lib/multipart/post/composite_read_io.rb#L80-L88

@ioquatix
Copy link
Member

Yes, something like this seems reasoanble.

I think if the path is not given, we can use nil. We could make it an optional keyword argument.

I am open to ideas, and agree the current implementation looks a bit odd.

@ts-3156
Copy link
Author

ts-3156 commented May 22, 2021

@ioquatix Thank you for your response!

I've created the most conservative diffs. Is this what you want?
master...ts-3156:set_file_as_default_filename

I think if the path is not given, we can use nil. We could make it an optional keyword argument.

Do you refer to local_path? As far as I tried, it's fine even if it is nil. However, I didn't include the change in that diffs because I'm not sure that not all gems that depend on the multipart-post gem will have the same result.


The code I am suggesting is very small. Besides, There's no need to rush. If you have a better idea, please feel free to close this issue without hesitation. It may be the easiest way for you.

@ioquatix
Copy link
Member

I think we should try to fix the interface.

The problem I have with filename = "file" unless filename is because we could legitimately have a file called "file". In this case, we can't distinguish the case where no file name was given or the file was actually called "file". If we do need some filename, then why not make a method, e.g.

DEFAULT_FILE_NAME = "unspecified.dat"

def filename(default: DEFAULT_FILE_NAME)
  @filename || default
end

@Lewiscowles1986 do you have any opinion?

@Lewiscowles1986
Copy link
Contributor

I would use nil which I read as indeterminate for filename so to avoid reserved (cursed) tokens that are hard to reason about; then internally set if nil. You could also use empty string via String.new as that is an invalid file name.

What do you think of this?

I believe most network clients like outlook, thunderbird, all the browsers default to attachment.dat

@ioquatix
Copy link
Member

I welcome a PR for this.

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

No branches or pull requests

3 participants