socketry / multipart-post

Adds multipart POST capability to net/http
MIT License
293 stars 72 forks source link

Refactor parts into multipart post namespace #65

Closed patrickdavey closed 5 years ago

patrickdavey commented 5 years ago

This is part of a refactoring series to move the top level modules (polluting the global namespace) to be nested under the MultipartPost namespace.

This PR moves the Parts module only. If this approach is acceptable, I'll work on the rest.

Thanks!

ioquatix commented 5 years ago

Thanks for this!

I think because of the name of this gem, the convention should be multipart/post/parts.rb and the namespace should be Multipart::Post::Parts.

What do you think?

Otherwise, we should rename the gem multipart_post but I think that boat has long since sailed.

patrickdavey commented 5 years ago

Ah, ok, Yip I can totally do that. I went with MultipartPost because that's what the lib/mutlipart_post.rb was using

Just so as I'm clear, the structure will become: lib/multipart_post.rb -> lib/multipart/post.rb, and I'll extract the version out of that into lib/multipart/post/version.rb

Want me to close this PR and open a new one? Or just force-push to this?

ioquatix commented 5 years ago

Force push is fine. Response to other points coming shortly.

ioquatix commented 5 years ago

Just so as I'm clear, the structure will become: lib/multipart_post.rb -> lib/multipart/post.rb

Yes, just make subdirectories to match modules.

I also think the deeply nested Parts module could perhaps be simplified, but I'll leave that up to you to think about. Feel free to make a proposal before doing it if you want feedback.

and I'll extract the version out of that into lib/multipart/post/version.rb

Yes, perfect.

ioquatix commented 5 years ago

What we can probably do for compatibility is have the old multipart_post.rb with a bunch of aliases/constants to match the old layout, along with a deprecation warning, e.g.

Parts = Multipart::Post::Parts

patrickdavey commented 5 years ago

Sorry that paused for a while, work has been somewhat busy for a bit :)

Anyway, let me know if you're happy with the current version. This PR should probably be closed & reopened / renamed at some point, as it's no longer about Parts (which was all I signed up for ;) ) heh. I've now removed (I think) all of the top-level polluting files out under the Multipart::Post namespace.

I'm not planning on refactoring/reworking the code itself, however, happy to try work in those deprecations. That's not something I've done before, so, I'll setup the aliases/constants [as you suggested(https://github.com/socketry/multipart-post/pull/65#issuecomment-540436358) and then probably do what's suggested on stack overflow (though that is more for methods than Constants... maybe there's another sneaky way?)

ioquatix commented 5 years ago

LGTM. Make further modifications on a new PR.

ioquatix commented 5 years ago

Please add your name to copyright in readme.