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

Features/operator redefinition #335

Closed

Conversation

amcguier
Copy link

Hi Folk,

This is a first stab at providing a slightly cleaner infix situation, referencing item #311. This cleans the namespace and allows other libraries to use monadic bind >>= in the standard way.

I'm pretty open to suggestion about this one. One alternative approach might be to have all the named prefix functions remain in the primary namespace, and move both my rebound infix operators, as well as the original Suave defined operators into separate modules where folk could use them (or not) without opening them into the global namespace by default.

That approach would require a little refactoring of the core modules to add an open statement for the default operators, but it might be the cleanest way to move forward.

Thoughts?

//Which frees other libraries to use >>= in the more tranditional way.


//Using a dummy type here so that our operator definition doesn't automatically smash
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this a real xmldoc comment?

Copy link
Author

Choose a reason for hiding this comment

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

Will do

@haf
Copy link
Contributor

haf commented Nov 17, 2015

"[...] move both my rebound infix operators, as well as the original Suave defined operators into separate modules where folk could use them (or not) without opening them into the global namespace by default."

But the operators aren't in the global namespace today, are they? They are in the Suave.Http module, and you can choose not to open it?

About the type member overloading; what would a downstream consumer override look like? You cannot attach more static members to the BindIt type, right? So instead you'd have to implement the inline operator on the type in question; but then you can't do that for framework types, like Choice, right?

@haf
Copy link
Contributor

haf commented Nov 17, 2015

We could do open Suave.Operators, perhaps? /cc @ademar @panesofglass @dsyme How do you think that affect the public API? Considering we'd be moving towards v1 API lockdown sometime now.

@panesofglass
Copy link
Contributor

As I recall, we used a child Operators module in F#x, where applicable. I don't think we did that for Freya, but we also moved away from having so many operators as a general rule. I don't know what's the right thing to do here, though. I don't personally mind having operators made available when I open a module.

@ademar
Copy link
Member

ademar commented Nov 18, 2015

@haf I am OK moving the operators to its own name space; do not know about the name though. The idea would be to phase them out eventually and promote usage of the symbols defined in this PR.

@ademar
Copy link
Member

ademar commented Nov 18, 2015

Perhaps use Suave.Operators for this module instead of HttpAltBindings.

@amcguier
Copy link
Author

If the group is willing to move the current operators into Http.Operators I'd be happy to close this, do that work, then open a new pull request. I like the idea of not opening the operators into the namespace by default, it seems like it makes things more pleasant for folk downstream and give them more flexibility.

If we did that, would we still want the redefinitions of the operators as I did here in a module, or would moving them out of the main Http namespace be sufficient?

@haf
Copy link
Contributor

haf commented Nov 19, 2015

So currently we have:

open Suave
open Suave.Http
open Suave.Types
...

as the default. With that change it would be this (for the default happy-path)

open Suave
open Suave.Http
open Suave.Types
open Suave.Operators
..

I think that is too much; if we perform this change, how about we make it AutoOpen in Suave and for the rest one would have to choose a submodule of the Sauve namespace? I.e.

open Suave // also opens Operators by default
// we want to start a web server from this place/module
open Suave.Web

I was also wondering if you @amcguier would consider answering my questions above?

@ademar
Copy link
Member

ademar commented Nov 19, 2015

We should spend some time thinking about the name spaces; I do not quite like the way they are now.

For example I feel that Suave.Http is a bad name.

@ademar
Copy link
Member

ademar commented Nov 19, 2015

Also Suave.Types types should probably inhabit the default name space.

@panesofglass
Copy link
Contributor

Agree about Suave.Types and also that you should minimize namespaces to open. I don't know about Suave.Http. Do you have any plans for opening up other networking protocols? What if I want to run a custom protocol over UDP? If you want to pursue that, then Suave.Http makes sense. Also, where does the Web Sockets and Server Sent Events live?

@ademar
Copy link
Member

ademar commented Nov 19, 2015

Also, where does the Web Sockets and Server Sent Events live?

I haven't made my mind yet but I'm thinking of moving them one level up. So we do.

open Suave.EventSource

Instead of.

open Suave.Http.EventSource

What do you think ? I would like to have less open statements in general.

@panesofglass
Copy link
Contributor

@ademar, well, EventSource is really part of HTTP, so I would think it sort of stays put, but I could also see the argument that one may want to only use EventSource and not the rest of HTTP. Really sort of depends on how it's built out, I think. If you don't need the rest of Http, bump it up.

@amcguier
Copy link
Author

@haf Sorry, I kind of lost them in the ongoing discussion.

  1. You're right, they're not open into the global namespace. However, there are lots of commonly functions in the Http namespace that you otherwise have to access in a fully qualified manner which means adding a module alias prefix every time you want to access one if you don't want the operators. Honestly I don't have strong feelings about this one way or another.
  2. I put the BindIt trick in there more as a novelty example. In this case it probably isn't as useful. That little hack around is useful if the operator is already defined into your namespace because you can add an overload without masking the existing implementation. It's most useful when someone has already defined the operator you're trying to use and you want to preserve their definition and add your own without masking the original operator. A downstream consumer could repeat the same trick and fall back through to the Suave definition, assuming they opened it into their namespace.
  3. As far as not having the additional namespace I can see that. You don't want it to be too complicated. I think your idea of having them auto-open into Suave is a good compromise as long as there aren't other types that are essential to consuming Suave that are only accessible from that namespace.

Did I get them all? If not, let me know. :-)

@haf
Copy link
Contributor

haf commented Nov 20, 2015

Ref #340

@haf haf added the apichange label Nov 21, 2015
@haf haf added this to the v1 milestone Nov 21, 2015
@haf
Copy link
Contributor

haf commented Nov 21, 2015

Ref #342

@amcguier
Copy link
Author

@haf If the operators are going to get moved into a namespace that makes it easy not to open them into the project namespace, I can just kill this PR and use that change. I'm too stuck on the changes I made anyway.

Thoughts?

@haf
Copy link
Contributor

haf commented Nov 24, 2015

@amcguier Yes, alright. Let's go for the namespace refactor PR. You're welcome to chime in on the refactor branch.

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.

4 participants