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

Consolidate Plug Locations #146

Merged

Conversation

jherdman
Copy link
Contributor

Move these two plugs with their other friends.

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

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

If we are going to move the files we should update the module names to reflect this change.

@jherdman
Copy link
Contributor Author

Do you have a recommendation? It appears that other plugs in this directory are named as such:

  • JSONAPI.ContentTypeNegotiation
  • JSONAPI.EnsureSpec
  • JSONAPI.FormatRequired
  • JSONAPI.IdRequired

I'm assuming you want something like JSONAPI.Plugs.MyPlug. If this is the case, is it possible to alias module names so as not to break existing code? We could then deprecate the old invocation.

@doomspork
Copy link
Member

I was mostly referring to JSONAPI.PlugResponseContentType, the Plug prefix is unnecessary. If we're going to move the files now would be the time to fix module names otherwise there's not much reason to move things around at this time, it's just churn.

I'm assuming you want something like JSONAPI.Plugs.MyPlug.

I personally would for clarity, but I'd like to hear what @jeregrine and @snewcomer think

If this is the case, is it possible to alias module names so as not to break existing code?

I don't think we want to start creating alias for backwards compatibility, maintaining that long term would be a nightmare. It's possible to cut a new release with a new version to signify breaking changes.

@jherdman
Copy link
Contributor Author

jherdman commented Dec 19, 2018

I don't think we want to start creating alias for backwards compatibility, maintaining that long term would be a nightmare. It's possible to cut a new release with a new version to signify breaking changes.

Agreed. It'd be nice to do this if even just to give people a fair heads up (if even until the 1.0 release).

I was mostly referring to JSONAPI.PlugResponseContentType, the Plug prefix is unnecessary...

Fixing!

Edit: I'm going to hold off on this until someone chimes in on backward compatibility.

Move these two plugs with their other friends.

Note that this commit introduces a backwards incompatible change: it
renames the `PlugResponseContentType` to `ResponseContentType`.
@jherdman
Copy link
Contributor Author

@doomspork after some silence on the matter I decided to make the changes you requested.

@@ -0,0 +1,27 @@
defmodule JSONAPI.ResponseContentTypeTest do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doomspork FYI added a test for this plug too, it was missing one.

@doomspork
Copy link
Member

@jherdman sorry! The run up the holidays is hectic around here.

@doomspork doomspork merged commit 86f75ef into beam-community:master Dec 24, 2018
jherdman added a commit to jherdman/jsonapi that referenced this pull request Dec 30, 2018
In beam-community#146 the `QueryParser` Plug was moved alongside its friends.
Unfortunately its test wasn't moved too. Let's fix that.
@jherdman jherdman mentioned this pull request Dec 30, 2018
doomspork pushed a commit that referenced this pull request Dec 30, 2018
In #146 the `QueryParser` Plug was moved alongside its friends.
Unfortunately its test wasn't moved too. Let's fix that.
@jherdman jherdman deleted the consolidate-plug-locations branch January 8, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants