Skip to content

Fix for static interfaces with non-static methods #1

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

Merged
merged 3 commits into from
Oct 12, 2015
Merged

Fix for static interfaces with non-static methods #1

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
153bca0
fix for static interfaces with non-static methods
zhengbli Oct 8, 2015
4aade26
CR feedback
zhengbli Oct 11, 2015
c88053e
CR feedback
zhengbli Oct 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 73 additions & 40 deletions TS.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,9 @@ let DumpCallBackFunctions flavor =
|> Array.iter DumpCallBackFunction

let DumpEnums () =
let DumpEnum (e: Browser.Enum) =
let dumpEnum (e: Browser.Enum) =
Pt.printl "declare var %s: string;" e.Name
browser.Enums |> Array.iter DumpEnum
browser.Enums |> Array.iter dumpEnum

/// Dump the properties and methods of a given interface
let DumpMembers flavor prefix (dumpScope: DumpScope) (i:Browser.Interface) =
Expand All @@ -244,7 +244,7 @@ let DumpMembers flavor prefix (dumpScope: DumpScope) (i:Browser.Interface) =
| _ -> DomTypeToTsType p.Type
Pt.printl "%s%s: %s;" prefix p.Name pType

if dumpScope <> DumpScope.StaticOnly then
if dumpScope <> StaticOnly then
match i.Properties with
| Some ps ->
ps.Properties
Expand Down Expand Up @@ -490,52 +490,85 @@ let DumpInterface flavor (i:Browser.Interface) =
Pt.printl ""

let DumpStaticInterface flavor (i:Browser.Interface) =
Pt.resetIndent()
DumpInterfaceDeclaration i
Pt.increaseIndent ()
// Some types are static types with non-static members. For example,
// NodeFilter is a static method itself, however it has an "acceptNode" method
// that expects the user to implement.
let hasNonStaticMember =
match i.Methods with
| Some ms -> ms.Methods |> Array.exists (fun m -> m.Static.IsNone)
Copy link
Member

Choose a reason for hiding this comment

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

the code base as a whole would benefit from additional Option methods like Option.toUnit and Option.toBool to give defaults when the properties are None. For example:

let toBool f = function
| Some x -> f x
| None -> false

This is general feedback, not for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if I'm understanding it correctly, would the code become:

match i.Methods with
| Some ms -> ms.Methods |> Array.exists (fun m -> Option.toBool m.Static)

Which seems to be similar?

Copy link
Member

Choose a reason for hiding this comment

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

No, it would be a top-level call:

i.Methods |> Option.toBool (fun ms -> ms.Methods |> Array.exists (fun m -> m.Static.IsNone))

This cuts a ton of repetitive three-line snippets down to one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks!

Edit: though ms.Methods is itself an Option<Browser.Method[]>, so the pipeline doesn't work

| _ -> false

let prefix = ""
DumpMembers flavor prefix DumpScope.StaticOnly i
DumpConstants i
DumpEventHandlers prefix i
let dumpStaticInterfaceWithNSMembers () =
Copy link
Member

Choose a reason for hiding this comment

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

Can you spell out NS? I thought it meant namespace (I missed the NonStatic just above ...)

Pt.resetIndent()
Copy link
Member

Choose a reason for hiding this comment

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

nit: decide on f() or f () and use only that. (I don't know what is the usual F# style.)

Copy link
Member

Choose a reason for hiding this comment

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

I usually use f () at declaration sites and f() at call sites. Just my input.

DumpInterfaceDeclaration i
Pt.increaseIndent ()

let dumpIndexers (ms:Browser.Method []) =
ms
|> Array.filter (fun m -> m.Getter.IsSome)
|> Array.iter
(fun m ->
let indexer = m.Params.[0]
Pt.printl "[%s: %s]: %s;"
indexer.Name
(DomTypeToTsType indexer.Type)
(DomTypeToTsType indexer.Type))
let prefix = ""
DumpMembers flavor prefix DumpScope.InstanceOnly i
DumpEventHandlers prefix i

// The indices could be within either Methods or Anonymous Methods
match i.Methods with
| Some ms -> ms.Methods |> dumpIndexers
| None -> ()
Pt.decreaseIndent()
Pt.printl "}"
Pt.printl ""
Pt.printl "declare var %s: {" i.Name
Pt.increaseIndent()
Copy link
Member

Choose a reason for hiding this comment

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

You may want to consider using a helper function like

let indentFor dump =
    Pt.increaseIndent()
    dump()
    Pt.decreaseIndent()

Also, "dump" is such an unappealing word. Maybe emit would be better if you're up to making that change in the future 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then how should I use indentFor? It seems then I need to wrap up the arbitrary print statements to a function then pass it to the indentFor everytime, which might be verbose as well?

Copy link
Member

Choose a reason for hiding this comment

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

It prevents you from forgetting to decreaseIndent each time. It's only a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Maybe something like the with keyword or using in C#. I'll look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW the word Dump is because it was called so in the old script :) Emit does sound better

Pt.printl "prototype: %s;" i.Name
DumpConstants i
DumpMembers flavor prefix DumpScope.StaticOnly i
Pt.decreaseIndent()
Pt.printl "}"
Pt.printl ""

match i.AnonymousMethods with
| Some ms -> ms.Methods |> dumpIndexers
| None -> ()
let dumpPureStaticInterface () =
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why these two functions are so different. How does the non-static variant get away without emitting indices?

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, I think the code would be easier to read if there were a single nested function that took a parameter or two (hasNonStaticMember ?), instead of two functions with lots of duplicated code.

Pt.resetIndent()
DumpInterfaceDeclaration i
Pt.increaseIndent ()

Pt.decreaseIndent()
Pt.printl "}"
Pt.printl "declare var %s: %s;" i.Name i.Name
Pt.printl ""
let prefix = ""
DumpMembers flavor prefix DumpScope.StaticOnly i
DumpConstants i
DumpEventHandlers prefix i

let dumpIndexers (ms:Browser.Method []) =
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this out of dumpPureStaticInterface, or at least to the top?

  1. It makes dumpStaticInterfaceWithNSMembers look quite different from dumpPureStaticInterface, even though they are quite similar.
  2. It increases the distance between Pt.increaseIndent () / Pt.decreaseIndent (), which is just another thing to push onto my mental stack when reading. (I like @DanielRosenwasser 's withIndent approach to help with this too.)

ms
|> Array.filter (fun m -> m.Getter.IsSome)
Copy link
Member

Choose a reason for hiding this comment

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

Based on @DanielRosenwasser 's earlier suggestion, you could perhaps write

Seq.filter (Browser.Method.Getter >> Option.isSome)

which I'm not sure is better.

(if F# supports first-class methods now, I know they were planning too)

Copy link
Member

Choose a reason for hiding this comment

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

I think the original is preferable

|> Array.iter
(fun m ->
let indexer = m.Params.[0]
Pt.printl "[%s: %s]: %s;"
indexer.Name
(DomTypeToTsType indexer.Type)
Copy link
Member

Choose a reason for hiding this comment

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

cache the tsType so you don't call this twice

(DomTypeToTsType indexer.Type))

// The indices could be within either Methods or Anonymous Methods
match i.Methods with
| Some ms -> ms.Methods |> dumpIndexers
| None -> ()

match i.AnonymousMethods with
| Some ms -> ms.Methods |> dumpIndexers
| None -> ()

Pt.decreaseIndent()
Pt.printl "}"
Pt.printl "declare var %s: %s;" i.Name i.Name
Pt.printl ""

if hasNonStaticMember then dumpStaticInterfaceWithNSMembers() else dumpPureStaticInterface()

let DumpNonCallbackInterfaces flavor =
GetNonCallbackInterfacesByFlavor flavor
|> Array.iter
Copy link
Member

Choose a reason for hiding this comment

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

I honestly think a for loop would look nicer than most uses of Array.iter here - right tool for the right job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A matter of preference then. I agree that here it might be cleaner to use for loop, but in some place it is at the end of a pipe chain, it is easier to continue the pipe instead of wrap it up with a loop.

Copy link
Member

Choose a reason for hiding this comment

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

What's the point of piping if you're only going to do it once? Seems like you're piping for the sake of piping. You could just do this:

for i in GetNonCallbackInterfacesByFlavor flavor do
    if i.Static.IsSome
        DumpStaticInterface flavor i
    elif i.NoInterfaceObject.IsSome
        DumpInterface flavor i
    else
        DumpInterface flavor i
        DumpConstructor flavor i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree. My point was made for "for loop is nicer than most uses of Array.iter" that you said above, not just here. I think a for loop is indeed cleaner in this particular place.

(fun i -> match i with
// Static attribute means singleton object
| i when i.Static.IsSome ->
DumpStaticInterface flavor i
| i when i.NoInterfaceObject.IsSome ->
DumpInterface flavor i
| _ ->
DumpInterface flavor i
DumpConstructor flavor i)
(fun i ->
match i with
// Static attribute the type doesn't have a constructor function
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify what you mean by this comment?

| i when i.Static.IsSome ->
Copy link
Member

Choose a reason for hiding this comment

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

Is if/elif/else not less verbose than this?

DumpStaticInterface flavor i
| i when i.NoInterfaceObject.IsSome ->
DumpInterface flavor i
| _ ->
DumpInterface flavor i
DumpConstructor flavor i)

let DumpDictionaries flavor =
let DumpDictionary (dict:Browser.Dictionary) =
Expand Down