Skip to content

Detect changes in json files regarding dictionary types #38

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 2 commits into from
Jan 11, 2016
Merged

Detect changes in json files regarding dictionary types #38

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de596be
Detect changes in json files regarding dictionary types
zhengbli Jan 6, 2016
f18a8dd
Change `Array.contains` to `Set.contains`
zhengbli Jan 9, 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
20 changes: 19 additions & 1 deletion TS.fsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,26 @@ let EmitDictionaries flavor =
| "Object" -> Pt.printl "interface %s {" dict.Name
| _ -> Pt.printl "interface %s extends %s {" dict.Name dict.Extends

let emitJsonProperty (p: ItemsType.Root) =
Pt.printl "%s: %s;" p.Name.Value p.Type.Value

let removedPropNames =
getRemovedItems ItemKind.Property flavor
|> Array.filter (matchInterface dict.Name)
|> Array.map (fun rp -> rp.Name.Value)
Copy link
Member

Choose a reason for hiding this comment

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

I would tack on

|> Set.ofArray

to make the contains check below faster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which contains check?

Copy link
Member

Choose a reason for hiding this comment

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

(fun m -> not (Array.contains m.Name removedPropNames)

I would change Array.contains to Set.contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is Set.contains faster than Array.contains? My impression is the opposite, because at least Set.contains has an implicit conversion from Array to Set.

Copy link
Member

Choose a reason for hiding this comment

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

Set.contains doesn't do any implicit conversions. You'd pay the construction cost once when building removedPropNames by adding |> Set.ofArray.

Generally, Set.contains is O(lg n) or O(1) and Array.contains is O(n), so if removedPropNames is ever long-ish (10-100 or more items probably?) it will be worth it.

However, I can't find any documentation on Array.contains! Where does Array.contains come from? I see that Shared.fs adds Seq.contains x = Seq.exists ((=)x), which is O(n), but I can't find a definition of Array.contains in our own code.

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. For some reason I was thinking about Seq instead of Set, therefore had the impression that Array.contains is faster. Will update soon.

I believe Array.contains is added in F# 4.0 which was not very well documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

|> Set.ofArray
let addedProps =
getAddedItems ItemKind.Property flavor
|> Array.filter (matchInterface dict.Name)

Pt.increaseIndent()
dict.Members |> Array.iter (fun m -> Pt.printl "%s?: %s;" m.Name (DomTypeToTsType m.Type))
Array.iter emitJsonProperty addedProps
dict.Members
|> Array.filter (fun m -> not (Set.contains m.Name removedPropNames))
|> Array.iter (fun m ->
match (findOverriddenItem m.Name ItemKind.Property dict.Name) with
| Some om -> emitJsonProperty om
| None -> Pt.printl "%s?: %s;" m.Name (DomTypeToTsType m.Type))
Copy link
Member

Choose a reason for hiding this comment

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

there's always multiple way to do things in F#. My personal style preference would be to do

let isRemovedPropName =
    let removedPropNames = ...
    not << fun m -> Set.contains m.Name removedPropNames
(* ... find and emit addedProps ... *)
for m in Array.filter isRemovedPropName dict.Members do
  (* ... same match body as before ... *)

This makes the side-effecting syntactically obvious and reduces the overhead from F#'s lambda syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is more or less subjective when it comes to the syntax preferences though. I used the continuation style because it makes the data flow clearer, and avoids the burden of naming things. Though I agree both styles have merits.

Pt.decreaseIndent()
Pt.printl "}"
Pt.printl ""
Expand Down