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

Move HTTP::Multipart to MIME::Multipart #7085

Merged
merged 2 commits into from
Feb 8, 2019
Merged

Move HTTP::Multipart to MIME::Multipart #7085

merged 2 commits into from
Feb 8, 2019

Conversation

m1lt0n
Copy link
Contributor

@m1lt0n m1lt0n commented Nov 14, 2018 •
edited
Loading

This is implementing the proposal of issue #7078

@m1lt0n m1lt0n force-pushed the move_multipart_and_formdata_to_mime branch 3 times, most recently from cc338fd to 67c46f1 Compare November 14, 2018 07:24
@m1lt0n m1lt0n changed the title Move HTTP::Formdata and HTTP::Multipart to Mime namespace Move HTTP::FormData and HTTP::Multipart to Mime namespace Nov 14, 2018
@ysbaddaden
Copy link
Contributor

I'd never expect MIME::FormData. I'd expect HTTP::FormData.

@m1lt0n
Copy link
Contributor Author

m1lt0n commented Nov 14, 2018

@ysbaddaden Yep, agree, it's kind of subjective. I have started working on crystal projects lately and wanted to start contributed somehow, so I went for a quick win based on an open issue. It's totally up for discussion if this should be merged as is or with amendments :)

@straight-shoota
Copy link
Member

@m1lt0n I opened an issue instead of a PR so this can be discussed first. Lets please talk about the general proceeding in #7078 and then come back to this PR.

@m1lt0n
Copy link
Contributor Author

m1lt0n commented Nov 14, 2018

@straight-shoota Sure! That was my intention too. Sorry if the PR took some time from people to look at it. My intention was to have a PR ready (that's why I referenced it in the PR) if the issue discussion proceeds and gets accepted as is (or with some ammendments based on the discussion on the issue). Thanks for your feedback.

@straight-shoota
Copy link
Member

@m1lt0n Could you revert to HTTP::FormData as per #7078 ?

@m1lt0n m1lt0n force-pushed the move_multipart_and_formdata_to_mime branch 2 times, most recently from cd70035 to 25e9acf Compare December 10, 2018 15:12
@m1lt0n
Copy link
Contributor Author

m1lt0n commented Dec 10, 2018

@straight-shoota Ok, done. Moved the changes of FormData back to HTTP.

straight-shoota
straight-shoota reviewed Dec 10, 2018
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

src/http/formdata.cr needs to have require "mime/multipart" as well.

spec/std/mime/multipart/builder_spec.cr Outdated Show resolved Hide resolved
spec/std/mime/multipart/parser_spec.cr Outdated Show resolved Hide resolved
spec/std/mime/multipart_spec.cr Outdated Show resolved Hide resolved
src/mime.cr Outdated Show resolved Hide resolved
src/mime/multipart.cr Outdated Show resolved Hide resolved
straight-shoota
straight-shoota approved these changes Dec 10, 2018
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

👍

asterite
asterite approved these changes Dec 15, 2018
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Please check if this can be merged right now, it's a breaking change so probably not suitable for 0.27.1

@RX14 RX14 added this to the 0.28.0 milestone Dec 15, 2018
sdogruyol
sdogruyol approved these changes Dec 25, 2018
View reviewed changes
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @m1lt0n 👍

@vlazar
Copy link
Contributor

vlazar commented Jan 7, 2019

Maybe update PR title as HTTP::FormData is no longer moved to Mime:: namespace?

Move HTTP::FormData and HTTP::Multipart to Mime namespace -> Move HTTP::Multipart to MIME namespace

@straight-shoota straight-shoota changed the title Move HTTP::FormData and HTTP::Multipart to Mime namespace Move HTTP::Multipart to MIME::Multipart Jan 7, 2019
@bcardiff
Copy link
Member

bcardiff commented Feb 7, 2019

@m1lt0n there are some conflicts since the last release. Would you be willing to rebase & fix on master so this can be merged?

@m1lt0n m1lt0n force-pushed the move_multipart_and_formdata_to_mime branch from e054091 to 6b25e16 Compare February 8, 2019 06:44
straight-shoota
straight-shoota approved these changes Feb 8, 2019
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@straight-shoota straight-shoota merged commit c163a85 into crystal-lang:master Feb 8, 2019
@Indribell Indribell mentioned this pull request Feb 8, 2019
Closed
@yorci yorci mentioned this pull request Apr 19, 2019
Merged
vectorselector added a commit to vectorselector/mailer that referenced this pull request Nov 1, 2019
@vectorselector
update mock.cr for Crystal 0.31.1
032e930 
Updated mock.cr for Crystal 0.31.1 per Crystal API breaking change crystal-lang/crystal#7085
vectorselector added a commit to vectorselector/mailer that referenced this pull request Nov 1, 2019
@vectorselector
update mailgun.cr
e81c2eb 
Updated mailgun.cr for Crystal 0.31.1 per Crystal API breaking change crystal-lang/crystal#7085
@vectorselector vectorselector mentioned this pull request Nov 1, 2019
Open
vectorselector added a commit to vectorselector/mailer that referenced this pull request Nov 1, 2019
@vectorselector
Update mock.cr
3d448be 
Updated mock.cr for Crystal 0.31.1 per Crystal API breaking change crystal-lang/crystal#7085

sorry for 2 PRs. I don't know how to combine 2 edits on github...
@vectorselector vectorselector mentioned this pull request Nov 1, 2019
Open
This was referenced Jul 9, 2020
Merged
Merged
vectorselector added a commit to vectorselector/mailer that referenced this pull request Nov 6, 2020
@vectorselector
Update mock.cr
e6205f2 
Updated mock.cr for Crystal 0.31.1 per Crystal API breaking change crystal-lang/crystal#7085
vectorselector added a commit to vectorselector/mailer that referenced this pull request Nov 6, 2020
@vectorselector
Update mailgun.cr
e2451b1 
Updated mailgun.cr for Crystal 0.31.1 per Crystal API breaking change crystal-lang/crystal#7085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RX14 RX14 RX14 approved these changes

@sdogruyol sdogruyol sdogruyol approved these changes

@straight-shoota straight-shoota straight-shoota approved these changes

@asterite asterite asterite approved these changes

Assignees
No one assigned
Labels
breaking-change kind:refactor status:accepted topic:stdlib:networking
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
8 participants
@m1lt0n @ysbaddaden @straight-shoota @vlazar @bcardiff @asterite @sdogruyol @RX14