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.

Sign up for GitHub

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

Jump to bottom

Fixes for RFC 2231 #726

Merged
merged 9 commits into from Jun 13, 2015
Merged

Fixes for RFC 2231 #726

merged 9 commits into from Jun 13, 2015

Conversation

sigmavirus24
Copy link
Contributor

No description provided.

@tenderlove
Copy link
Member

Can you rebase this please? Thank you!

@sigmavirus24
Copy link
Contributor Author

On it

sigmavirus24 and others added 9 commits June 13, 2015 14:19
@sigmavirus24
Add failing test
c7d291f
@sigmavirus24
Start updating definitions for RFC 2231
b3e90ed
@sigmavirus24
Move RFC2183 constant too
12703d3
@sigmavirus24
Simplify regular expressions and extract the extended filename
5f5681d
@sigmavirus24
Feature: Provided support for non-ascii character in public header
b5e7ff4 
I love that we worked on this @madisonruby for #OSL
@sigmavirus24
Force encoding of parsed filename
2037507 
I love working at the #OSL
@sigmavirus24
FIX: Backwards compatibility with soupy data
57e030f 
* Broken quotes needs to be checked before RFC2231 otherwise the filenames are
  not correctly found

* Parsing "files" out of the header was converted to an empty string instead
  instead of nil when the body was empty.

I love working at the #OSL
@sigmavirus24
Use ::Encoding instead of Parser::Encoding
1613b02
@sigmavirus24
Fix encoding issue in source files
2d8fee3
@sigmavirus24
Copy link
Contributor Author

Done

tenderlove added a commit that referenced this pull request Jun 13, 2015
@tenderlove
Merge pull request #726 from sigmavirus24/rfc-2231
df05b3a 
Fixes for RFC 2231
@tenderlove tenderlove merged commit df05b3a into rack:master Jun 13, 2015
@sigmavirus24
Copy link
Contributor Author

✨ 🍰 ✨

@tenderlove
Copy link
Member

Thanks. Would you mind running the tests? This PR is generating warnings with the new regexps:

/Users/aaron/git/rack/lib/rack/multipart.rb:31: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:32: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:32: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:33: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:33: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:34: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:34: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:34: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:34: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:35: warning: nested repeat operator + and ? was replaced with '*'
/Users/aaron/git/rack/lib/rack/multipart.rb:35: warning: nested repeat operator + and ? was replaced with '*'

@sigmavirus24
Copy link
Contributor Author

That's interesting, I don't remember seeing those before. I'll take a look at them tonight. Sorry about that

@tenderlove
Copy link
Member

No problem! It might just be against trunk Ruby (Ruby 2.3)

@sigmavirus24
Copy link
Contributor Author

On Sat, Jun 13, 2015 at 03:36:31PM -0700, Aaron Patterson wrote:

No problem! It might just be against trunk Ruby (Ruby 2.3)

Nah, it shows up in Travis-CI for 2.2 too. I don't think I had 2.2 installed
last year when I worked on this.

@sigmavirus24
Copy link
Contributor Author

So I took a look at this tonight finally. The interesting thing is that on L31 of lib/rack/multipart.rb if I change

EXTENDED_INITIAL_VALUE = /(?:[a-zA-Z0-9\-]+)?'(?:[a-zA-Z0-9\-]+)?'(?:#{EXTENDED_OTHER_VALUE}+)/

to

EXTENDED_INITIAL_VALUE = /(?:#{EXTENDED_OTHER_VALUE}+)/

Those warnings disappear. I'm toying around in rubular to see if I can find an equivalent regular expression to make Ruby 2.2 happy.

@sigmavirus24
Copy link
Contributor Author

So, the warnings go away (but the tests fail) if we do:

EXTENDED_INITIAL_VALUE = /([a-zA-Z0-9\-]+)?'([a-zA-Z0-9\-]+)?'(#{EXTENDED_OTHER_VALUE}+)/

But in reality, we don't need to capture those matches individually.

@sigmavirus24 sigmavirus24 mentioned this pull request Jun 16, 2015
Closed
@sigmavirus24
Copy link
Contributor Author

@tenderlove here's the fix: #892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sigmavirus24 @tenderlove