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

Change casing convention (API review #1) #206

Merged
merged 30 commits into from Feb 15, 2015

Conversation

Projects
None yet
4 participants
@dsyme
Copy link
Contributor

dsyme commented Feb 12, 2015

As discussed in #204 there is a proposal from myself in conjunction with @haf and @ademar to change the naming convention used in Suave to be closer to F# standards.

This PR implements a change to camelCase for at least the majority of functions in Suave. Some are not done in the PR deliberately, e.g.

  • FOO_BAR (it's best to keep these with _)
  • foo_ identifiers that represent Lens-style functional-properties-as-values

This is a non-trivial issue since it's considerably invasive on the API. Numerous Obsolete's have been added (but it's possible Obsolete for some less common functions may have been missed)

Some other API design changes are made as well

  • foo_ become static members. Given the pattern being implemented they are intrinsically part of the type. In their use it seems reasonable to qualify them by type name since field names are not guaranteed to be unique in any case
  • Getter functions like SuaveConfig.bindings (as opposed to just using the "bindings" property) are removed. While it's OK in principle to have these, I don't recommend them in the library design - it looks better to normalize client code to use the properties for such fundamental properties of the types. Nearly all off them were unused in the Suave code itself.
  • HttpCode was moved out of "Suave.Types.Codes" and into "Suave.Types" and the associated functions made into members. That seems the right design
  • Nearly all ' functions have been renamed to clarify what the ' means. For example dir' becomes dirHomeDirectory.

I'd imagine this will generate a considerable amount of discussion and review questions. I'll add notes in the PR itself for further discussion points, please feel free to shoot away with lots of questions.

The docs have not yet been updated.

@@ -1,51 +1,51 @@
module CounterDemo

This comment has been minimized.

@dsyme

dsyme Feb 12, 2015

Contributor

I'm not sure why GitHub is marking these changes, perhaps a line-ending issue

Don Syme added some commits Feb 12, 2015

Don Syme
Don Syme
req_parent_id : uint64 option }
reqParentId : uint64 option }

static member traceId_ = Property (fun x -> x.traceId) (fun v x -> { x with traceId = v })

This comment has been minimized.

@kolektiv

kolektiv Feb 12, 2015

Contributor

Hope nobody minds me dropping a comment in here! This is quite an interesting one for me, as I've struggled to decide on a consistent approach to structuring lens properties/functions. At first glance static members seemed ideal, as the lens itself from Type -> Property of Type is intrinsically bound to that type.

However, when you start to define composite lenses, where should they live? They're no longer bound to a single type. How about lenses which aren't bound to a type in the first place? (For example, lenses for the fst and snd items of a 2-tuple). You also can't extend a type alias, so when you wish to define lenses on a specialisation of a generic type, that's not possible in the same way.

It's a tricky one, and I've ended up with a mix throughout the code I write using lenses. I'm still not really delighted by that as a general rule, but if anything i'm leaning back towards lenses always being effectively a naked function (or function pair, in this formulation). Slightly off topic perhaps, but as they're being commented on here, seemed a chance to see if others had strong opinions...

This comment has been minimized.

@dsyme

dsyme Feb 12, 2015

Contributor

I don't think any programming language has a really great place to put items ABC composed from arbitrary primitives A, B and C :)

Can you send a link to the design patterns you use for lenses (=functional settable properties) ? Perhaps also add comments on this thread detailing the design pattern you use: https://fslang.uservoice.com/forums/245727-f-language/suggestions/6906132-implement-first-class-lensing-lenses-in-f

This comment has been minimized.

@kolektiv

kolektiv Feb 12, 2015

Contributor

I use (and wrote) Aether (https://github.com/xyncro/aether) for lensing in F#. The API for that is slightly non-F# ish and I'm actually considering a pending breaking change to fix that, but how it works isn't too likely to change now. It's based on the fairly naive implementation of lenses as composable get,set pairs, which is about the only option in F# short of language level changes (which, to be honest, would either have to be very feature-specific, which would make me hesitate, or very high level and general and "change to the type system" which doesn't seem too likely at this point). The naive implementation does work well enough though, and makes working with complex data structures more tractable/feasible.

Here's a (slightly nasty, but internal!) example from Hekate: https://github.com/xyncro/hekate/blob/master/src/Hekate/Hekate.fs#L177-L183 - note that the lenses are defined away from the types anyway, as they apply to type aliases. Here's a slightly nicer example where lenses are used to provide simple access to a more complex structure under the guise of some functions: https://github.com/kolektiv/freya/blob/develop/src/Freya.Machine/Configuration.fs - in that example one of the lenses being composed is defined as a static member on the type (as that's possible), but it doesn't feel like a convention that quite fits when other lenses won't be able to be. Also, from the consumer perspective, whether a lens is composed or not is opaque - it's just a lens, so distinguishing them, when all can be composed, etc. seems slightly awkward.

(As an aside, I'm about 95% sold on changing the functions such as getL, modL, setPL, etc. to things like Lens.get, Lens.map, and Lens.setPartial respectively. Increased verbosity and typing, but it is rather more the "F# way" - I think! I've gone more in that direction with more recent libs...)

This comment has been minimized.

@kolektiv

kolektiv Feb 12, 2015

Contributor

Oh. as a slightly random add-on to that - the Haskell convention for lens naming (which obviously has rather different standards generally, but...) is usually the name of the property of the data-type, and a tick. So rather than my current usual standard of something like configLens it's more likely to just be config'

This comment has been minimized.

@dsyme

let getFormValue name =
request (fun x -> OK (HttpRequest.form x ^^ name |> Option.get))
request (fun x -> OK (x.queryParam name |> Option.get))

This comment has been minimized.

@ademar

ademar Feb 12, 2015

Member

I think here it should be

let getFormValue name =
    request (fun x -> OK (x.formDataItem name |> Option.get))

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Similarly to queryParam comment, should we just do x.formData? Less typing...?

return! async {
do! Async.Sleep 100
return Choice1Of2 () } }
let q = req.queryParams

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Should we just call it query? Does params add anything?

This comment has been minimized.

@dsyme

dsyme Feb 12, 2015

Contributor

I think it's related to the "cost of parsing" question further below

GET >>= dir' //show directory listing
basicAuth // from here on it will require authentication
GET >>= url "/events" >>= request (fun r -> EventSource.handShake (CounterDemo.counterDemo r))
GET >>= browseHomeDirectory //serves file if exists

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Doesn't browse already imply that it's a directory? What about browseHome?

This comment has been minimized.

@dsyme

dsyme Feb 13, 2015

Contributor

We'd have to change all of these:

val browseHomeDirectory : WebPart
val browseFileInHomeDirectory : fileName:string -> WebPart
val dirHomeDirectory : WebPart

to

val browseHome : WebPart
val browseFileInHome : fileName:string -> WebPart
val dirHome : WebPart

or

val browseHome : WebPart
val browseFileHome : fileName:string -> WebPart
val dirHome : WebPart

or

val browseHomeDir : WebPart
val browseFileHomeDir : fileName:string -> WebPart
val dirHomeDir : WebPart

I think the second option is best, I've applied it, let me know what you'd like.

mime_types_map = mime_types
home_folder = None
compressed_files_folder = None
serverKey = Utils.Crypto.generateKey HttpRuntime.ServerKeyLength

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Could we re-align this whitespace for readability?

@@ -9,16 +9,16 @@ open Suave.Http.Applicatives
open Suave.Http.Files
open Suave.Logging

let logger = Loggers.sane_defaults_for LogLevel.Verbose
let logger = Loggers.saneDefaultsFor LogLevel.Verbose

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

When we're changing things, are we happy with this name? I find it a bit 'shoot from the hip'...

This comment has been minimized.

@dsyme

dsyme Feb 12, 2015

Contributor

I reckon it could do with a change :)

This comment has been minimized.

@dsyme

dsyme Feb 13, 2015

Contributor

(Let's do this name change in a follow-up PR)

; max_ops = 10000
; logger = logger }
bufferSize = 2048
maxOps = 10000

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

What about using unsigned integers, unless they can go below zero?

This comment has been minimized.

@dsyme

dsyme Feb 13, 2015

Contributor

I think signed is ok, it's pretty normal, but either way let's do it in a follow up PR/discussion?


let title' attr s = tag "title" attr (Xml([Text s,Xml []]))
let title = title' Array.empty<Attribute>
let htmlAttr attr s = tag "html" (Array.ofList attr) s

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Wouldn't it be nicer to have Array.toList inside the tag function?

This comment has been minimized.

@dsyme

dsyme Feb 12, 2015

Contributor

yes, done, thanks

let sample_page =
html <% [
head <% [
let samplePage =

This comment has been minimized.

@haf

haf Feb 12, 2015

Contributor

Ping @ademar - changes to syntax, removal of <% special syntax.

Don Syme added some commits Feb 12, 2015

Don Syme
Don Syme
@haf

This comment has been minimized.

Copy link
Contributor

haf commented Feb 12, 2015

@ademar Could you make AppVeyor do gem install bundler before bundle exec rake, please?

Don Syme
@ademar

This comment has been minimized.

Copy link
Member

ademar commented Feb 12, 2015

@haf AppVeyor now hangs precisly on that line.

@ademar

This comment has been minimized.

Copy link
Member

ademar commented Feb 13, 2015

AppVeyor build is passing.

Don Syme added some commits Feb 13, 2015

Don Syme
Don Syme
@ademar

This comment has been minimized.

Copy link
Member

ademar commented Feb 13, 2015

@dsyme can we merge this ?

http_only : bool }
httpOnly : bool }

static member nameP = (fun x -> x.name), fun v x -> { x with name = v }

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

Mix between P and _ for the changed lenses.

let name_ =
(fun x -> x.name),
fun v (x : MimeType) -> { x with name = v }
static member name_ = Property (fun x -> x.name) (fun v (x : MimeType) -> { x with name = v })

This comment has been minimized.

@haf

let file_name x = x.file_name
static member fieldName_ = Property<HttpUpload,_> (fun x -> x.fieldName) (fun v x -> { x with fieldName = v })

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

Here

ipaddr : IPAddress }

static member httpVersion_ = Property<HttpRequest,_> (fun x -> x.httpVersion) (fun v (x : HttpRequest) -> { x with httpVersion = v })

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

Here

[<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module HttpRequest =

open Suave.Utils

let empty =
{ http_version = "1.1"
{ httpVersion = "1.1"

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

Whitespace




module (* internal *) Bytes =

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

+1 on making it internal

This comment has been minimized.

@dsyme

dsyme Feb 13, 2015

Contributor

I ran into trouble with BufferSegment being exposed in some public API (which can probably also be made internal). Let's use a separate PR for this whole "internal" issue.

bw.Flush ()

let hmac = hmac' key (cipher_text.ToArray())
let hmac = hmacOfBytes key (cipher_text.ToArray())

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

One more rename here

@@ -68,11 +76,11 @@ module ParsingAndControl =

/// Iterates over a BufferSegment list looking for a marker, data before the marker
/// is sent to the function select and the corresponding buffers are released
let scan_marker marker select connection = socket {
let scanMarker marker select connection = socket {

match kmp_z marker connection.segments with

This comment has been minimized.

@haf

haf Feb 13, 2015

Contributor

One more rename here

@haf

This comment has been minimized.

Copy link
Contributor

haf commented Feb 13, 2015

Looking good!

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Feb 14, 2015

I think this is almost ready to go.

The biggest question is about the naming and "parsing costs" for these:

request.queryParams
request.formData
request.headers
request.cookies

request.queryParam key
request.formDataItem key
request.header key
request.cookie key

See discussion here

First, the naming is a bit inconsistent.

Second, it seems that all of these should have their parsing costs amortized (the results of parsing computed on-demand and stored inside the request or response object), regardless of which methods/properties are used to access the information.

@ademar

This comment has been minimized.

Copy link
Member

ademar commented Feb 14, 2015

Yes we should give a simpler more similar names to these two:

request.queryParams
request.formData

Maybe just query and form

We could also discuss packing multipart_fields into formData since people usually expect them to be there

Second, it seems that all of these should have their parsing costs amortized ..

Shall we tackle this in another PR ? I think this topic is not trivial, one could argue that if you need to parse two times is probably because you are doing it wrong ( ref: the parse post data by default argument ). Note that the modified HttpContext (the one possibly containing the parsed data) does not trickle down along the next route.

@dsyme dsyme referenced this pull request Feb 14, 2015

Merged

update docs for API change #208

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Feb 14, 2015

Maybe just query and form

I don't think "query" works - it seems to convey the wrong meaning, especially in the context of F# web programming where the word "query" is so related to LINQ, query { ... } and jQuery.

However I've updated to use it for now, so we have

request.query
request.form
request.headers
request.cookies

request.queryParam key
request.formData key
request.header key
request.cookie key

I can see the problem with amortization.

@dsyme

This comment has been minimized.

Copy link
Contributor

dsyme commented Feb 14, 2015

OK, so I think this is ready

dsyme added some commits Feb 14, 2015

ademar added a commit that referenced this pull request Feb 15, 2015

Merge pull request #206 from dsyme/api-review-1
Change casing convention (API review #1)

@ademar ademar merged commit d0ed527 into SuaveIO:master Feb 15, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment