Skip to content

Detect added json items for static interfaces #52

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 4 commits into from
Jan 25, 2016
Merged

Detect added json items for static interfaces #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d14836
Detect added json items for static interfaces
Jan 19, 2016
4cbc76c
Merge branch 'master' of https://github.com/Microsoft/TSJS-lib-genera…
zhengbli Jan 22, 2016
07c0468
Address CR
zhengbli Jan 22, 2016
8021539
Accepting new baseline
zhengbli Jan 22, 2016
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
16 changes: 13 additions & 3 deletions Shared.fsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,19 @@ module JsonItems =
t.Kind.ToLower() = kind.ToString() &&
(t.Flavor.IsNone || t.Flavor.Value = flavor.ToString() || flavor = All))

let getOverriddenItems kind flavor = getItems overriddenItems kind flavor
let getAddedItems kind flavor = getItems addedItems kind flavor
let getRemovedItems kind flavor = getItems removedItems kind flavor
let getOverriddenItems kind flavor =
getItems overriddenItems kind flavor
let getAddedItems kind flavor =
getItems addedItems kind flavor
let getRemovedItems kind flavor =
getItems removedItems kind flavor
Copy link
Member

Choose a reason for hiding this comment

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

why not let these be the point-free versions, eg

let getAddedItems = getItems addedItems

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!


let getAddedItemsByInterfaceName kind flavor iName =
getItems addedItems kind flavor |> Array.filter (matchInterface iName)
let getOverriddenItemsByInterfaceName kind flavor iName =
getItems overriddenItems kind flavor |> Array.filter (matchInterface iName)
let getRemovedItemsByInterfaceName kind flavor iName =
getItems removedItems kind flavor |> Array.filter (matchInterface iName)
Copy link
Member

Choose a reason for hiding this comment

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

why not use the pre-composed versions you just added? eg instead of getItems removedItems say getRemovedItems


module Comments =
type CommentType = JsonProvider<"inputfiles/comments.json">
Expand Down
31 changes: 24 additions & 7 deletions TS.fsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,17 +465,33 @@ let EmitStaticInterface flavor (i:Browser.Interface) =
// 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)
| _ -> false
let hasNonStaticMember =
let hasNonStaticMethod =
match JsonItems.getAddedItemsByInterfaceName ItemKind.Method flavor i.Name with
| [||] ->
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.

why don't you have to also check m.Static.Value like in the added methods case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original idea was the there is no static property, the only thing similar may be a static constant, which should be added as constant in the json file because it has a value. Though you may be right that it is better checking here to be conservative.

| _ -> false
| addedMs -> addedMs |> Array.exists (fun m -> m.Static.IsNone || m.Static.Value = false)
Copy link
Member

Choose a reason for hiding this comment

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

In the added methods case, don't you also need to check the non-overridden methods in i.Methods?

let hasProperty =
if i.Properties.IsSome then true
else
Copy link
Member

Choose a reason for hiding this comment

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

if x then true else y is equivalent to x || y

JsonItems.getAddedItemsByInterfaceName ItemKind.Property flavor i.Name |> Array.isEmpty |> not
hasNonStaticMethod || hasProperty

let emitAddedConstructor () =
match JsonItems.getAddedItemsByInterfaceName ItemKind.Constructor flavor i.Name with
| [||] -> ()
| ctors ->
Pt.printl "prototype: %s;" i.Name
ctors |> Array.iter (fun ctor -> ctor.Signatures |> Array.iter (Pt.printl "%s;"))
Copy link
Member

Choose a reason for hiding this comment

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

You can save a couple lines and some visual noise by getting rid of the match:

let ctors = JsonItems....
if not <| Array.isEmpty ctors then Pt.printl ...
ctors |> Array.iter ...


// For static types with non-static members, we put the non-static members into an
// interface, and put the static members into the object literal type of 'declare var'
// For static types with only static members, we put everything in the interface.
// Because in the two cases the interface contains different things, it might be easier to
// read to seperate them into two functions.
let emitStaticInterfaceWithNonStaticMembers () =
let emitStaticInterfaceWithNonStaticMembers () =
Pt.resetIndent()
EmitInterfaceDeclaration i
Pt.increaseIndent()
Expand All @@ -492,6 +508,7 @@ let EmitStaticInterface flavor (i:Browser.Interface) =
Pt.increaseIndent()
EmitConstants i
EmitMembers flavor prefix EmitScope.StaticOnly i
emitAddedConstructor ()
Pt.decreaseIndent()
Pt.printl "}"
Pt.printl ""
Expand All @@ -506,7 +523,7 @@ let EmitStaticInterface flavor (i:Browser.Interface) =
EmitConstants i
EmitEventHandlers prefix i
EmitIndexers EmitScope.StaticOnly i

emitAddedConstructor ()
Copy link
Member

Choose a reason for hiding this comment

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

when does a pure static interface have a constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens when the spec wrongly marked an interface as pure static, and a constructor is added in the json files. For example microsoft/TypeScript#2583

Pt.decreaseIndent()
Pt.printl "}"
Pt.printl "declare var %s: %s;" i.Name i.Name
Expand Down Expand Up @@ -616,4 +633,4 @@ let EmitDomWeb () =

let EmitDomWorker () =
ignoreDOMTypes <- true
EmitTheWholeThing Flavor.Worker GlobalVars.tsWorkerOutput
EmitTheWholeThing Flavor.Worker GlobalVars.tsWorkerOutput