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

Fixes and copyDynamicProperties #27

Merged
merged 5 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
66 changes: 55 additions & 11 deletions src/DynamicObj/DynamicObj.fs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,8 @@ type DynamicObj() =
/// Gets property value
member this.TryGetValue name =
Copy link
Member

Choose a reason for hiding this comment

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

Does this fix #25 ? If so, please add a regression test

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, not related

// first check the Properties collection for member
match properties.TryGetValue name with
| true,value -> Some value
// Next check for Public properties via Reflection
| _ -> ReflectionUtils.tryGetPropertyValue this name

this.TryGetPropertyInfo(name)
|> Option.map (fun pi -> pi.GetValue(this))

member this.GetValue (name) =
this.TryGetValue(name).Value
Expand All @@ -39,12 +36,43 @@ type DynamicObj() =
| :? 'a -> o :?> 'a |> Some
| _ -> None


member this.TryGetStaticPropertyInfo name : PropertyHelper option =
ReflectionUtils.tryGetStaticPropertyInfo this name

member this.TryGetDynamicPropertyInfo name : PropertyHelper option =
#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT
FableJS.tryGetDynamicPropertyHelper this name
#endif
#if FABLE_COMPILER_PYTHON
FablePy.tryGetDynamicPropertyHelper this name
#endif
#if !FABLE_COMPILER
match properties.TryGetValue name with
| true,_ ->
Some {
Name = name
IsStatic = false
IsDynamic = true
IsMutable = true
IsImmutable = false
GetValue = fun o -> properties.[name]
SetValue = fun o v -> properties.[name] <- v
RemoveValue = fun o -> properties.Remove(name) |> ignore
}
| _ -> None
#endif

member this.TryGetPropertyInfo name : PropertyHelper option =
match this.TryGetStaticPropertyInfo name with
| Some pi -> Some pi
| None -> this.TryGetDynamicPropertyInfo name

/// Sets property value, creating a new property if none exists
member this.SetValue (name,value) = // private
// first check to see if there's a native property to set

match ReflectionUtils.tryGetPropertyInfo this name with
match ReflectionUtils.tryGetStaticPropertyInfo this name with
Copy link
Member

Choose a reason for hiding this comment

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

Now that you have this.TryGetStaticPropertyInfo, can you use it here instead of an external module?

Copy link
Member Author

Choose a reason for hiding this comment

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

What module do you mean?

Copy link
Member

@kMutagene kMutagene Aug 30, 2024

Choose a reason for hiding this comment

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

ReflectionUtils.tryGetStaticPropertyInfo is used although a method with the exact same name exists on the class. On other members, you used the instance method instead of the functions from ReflectionUtils

Copy link
Member

Choose a reason for hiding this comment

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

For example, you used this.TryGetPropertyInfo in the Remove method instead of ReflectionUtils.tryGetPropertyInfo

| Some pi ->
if pi.IsMutable then
pi.SetValue this value
Expand All @@ -65,11 +93,10 @@ type DynamicObj() =
#endif

member this.Remove name =
match ReflectionUtils.removeProperty this name with
| true -> true
// Maybe in map
| false -> properties.Remove(name)

match this.TryGetPropertyInfo name with
| Some pi when pi.IsMutable -> pi.RemoveValue this
| Some _ -> failwith $"Cannot remove value for static, immutable property \"{name}\""
| None -> ()

member this.GetPropertyHelpers (includeInstanceProperties) =
#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT
Expand Down Expand Up @@ -137,6 +164,23 @@ type DynamicObj() =
#endif
|> Seq.filter (fun kv -> kv.Key.ToLower() <> "properties")

/// Copies all dynamic members of the DynamicObj to the target DynamicObj.
member this.CopyDynamicPropertiesTo(target:#DynamicObj, ?overWrite) =
let overWrite = Option.defaultValue false overWrite
Copy link
Member

Choose a reason for hiding this comment

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

There is also defaultArg, i vaguely remember that being better to use in these scenarios, but i do not remember why. Your call

Copy link
Member Author

Choose a reason for hiding this comment

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

For Fable you mean?

Copy link
Member

@kMutagene kMutagene Aug 30, 2024

Choose a reason for hiding this comment

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

no i mean in general for determining default args for optional arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

I think here it doesn't matter.

In cases where the default expression is computationally expensive, it's worth to switch to an explicit match. But not sure whether Option.defaultValue and defaultArg even have different implementations.

this.GetProperties(false)
|> Seq.iter (fun kv ->
match target.TryGetPropertyInfo kv.Key with
| Some pi when overWrite -> pi.SetValue target kv.Value
| Some _ -> failwith $"Property \"{kv.Key}\" already exists on target object and overWrite was not set to true."
| None -> target.SetValue(kv.Key,kv.Value)
)

/// Returns a new DynamicObj with only the dynamic properties of the original DynamicObj (sans instance properties).
member this.CopyDynamicProperties() =
let target = DynamicObj()
this.CopyDynamicPropertiesTo(target)
target

member this.GetPropertyNames(includeInstanceProperties) =
this.GetProperties(includeInstanceProperties)
|> Seq.map (fun kv -> kv.Key)
Expand Down
90 changes: 50 additions & 40 deletions src/DynamicObj/FableJS.fs
Original file line number Diff line number Diff line change
Expand Up @@ -85,58 +85,68 @@ module FableJS =
getPropertyValue o propName

[<Emit("Object.getOwnPropertyDescriptor($0, $1)")>]
let getPropertyDescriptor (o:obj) (propName:string) =
let tryGetPropertyDescriptor (o:obj) (propName:string) : obj option =
jsNative

let getStaticPropertyDescriptor (o:obj) (propName:string) =
getPropertyDescriptor (getPrototype o) propName

let tryGetStaticPropertyDescriptor (o:obj) (propName:string) : obj option=
tryGetPropertyDescriptor (getPrototype o) propName

let tryStaticPropertyHelperFromDescriptor (pd:obj) (name:string) : PropertyHelper option =
let isWritable = PropertyDescriptor.isWritable pd
if PropertyDescriptor.isFunction pd then
None
else
{
Name = name
IsStatic = true
IsDynamic = false
IsMutable = isWritable
IsImmutable = not isWritable
GetValue = createGetter name
SetValue = createSetter name
RemoveValue = createRemover name true
}
|> Some

let tryGetStaticPropertyHelper (o:obj) (propName:string) : PropertyHelper option =
tryGetStaticPropertyDescriptor o propName
|> Option.bind (fun pd -> tryStaticPropertyHelperFromDescriptor pd propName)

let getStaticPropertyHelpers (o:obj) : PropertyHelper [] =
getStaticPropertyNames o
|> Array.choose (fun n ->
let pd = getStaticPropertyDescriptor o n
if PropertyDescriptor.isFunction pd then
None
else
let isWritable = PropertyDescriptor.isWritable pd
{
Name = n
IsStatic = true
IsDynamic = false
IsMutable = isWritable
IsImmutable = not isWritable
GetValue = createGetter n
SetValue = createSetter n
RemoveValue = createRemover n true
}
|> Some
)
|> Array.choose (tryGetStaticPropertyHelper o)

let tryGetDynamicPropertyDescriptor (o:obj) (propName:string) : obj option =
tryGetPropertyDescriptor o propName

let transpiledPropertyRegex = "^[a-zA-Z]+@[0-9]+$"

let isTranspiledPropertyHelper (propertyName : string) =
System.Text.RegularExpressions.Regex.IsMatch(propertyName, transpiledPropertyRegex)

let tryDynamicPropertyHelperFromDescriptor (pd:obj) (name:string) : PropertyHelper option =
if PropertyDescriptor.isFunction pd || isTranspiledPropertyHelper name then
None
else
{
Name = name
IsStatic = false
IsDynamic = true
IsMutable = true
IsImmutable = false
GetValue = createGetter name
SetValue = createSetter name
RemoveValue = createRemover name false
}
|> Some

let tryGetDynamicPropertyHelper (o:obj) (propName:string) : PropertyHelper option =
tryGetDynamicPropertyDescriptor o propName
|> Option.bind (fun pd -> tryDynamicPropertyHelperFromDescriptor pd propName)

let getDynamicPropertyHelpers (o:obj) : PropertyHelper [] =
getOwnPropertyNames o
|> Array.choose (fun n ->
let pd = getPropertyDescriptor o n
if PropertyDescriptor.isFunction pd || isTranspiledPropertyHelper n then
None
else
let isWritable = PropertyDescriptor.isWritable pd
{
Name = n
IsStatic = false
IsDynamic = true
IsMutable = isWritable
IsImmutable = not isWritable
GetValue = createGetter n
SetValue = createSetter n
RemoveValue = createRemover n false
}
|> Some
)
|> Array.choose (tryGetDynamicPropertyHelper o)

let getPropertyHelpers (o:obj) =
getDynamicPropertyHelpers o
Expand Down
2 changes: 1 addition & 1 deletion src/DynamicObj/FablePy.fs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ module FablePy =



[<Emit("$0.__dict__[$1]")>]
[<Emit("$0.__dict__.get($1)")>]
let getMemberObject (o:obj) (propName:string) =
nativeOnly

Expand Down
16 changes: 8 additions & 8 deletions src/DynamicObj/ReflectionUtils.fs
Original file line number Diff line number Diff line change
Expand Up @@ -24,28 +24,28 @@ module ReflectionUtils =
#endif

/// Try to get the PropertyInfo by name using reflection
let tryGetPropertyInfo (o:obj) (propName:string) =
let tryGetStaticPropertyInfo (o:obj) (propName:string) =
#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT
FableJS.getPropertyHelpers o
FableJS.tryGetStaticPropertyHelper o propName
#endif
#if FABLE_COMPILER_PYTHON
FablePy.getPropertyHelpers o
FablePy.tryGetStaticPropertyHelper o propName
#endif
#if !FABLE_COMPILER
getStaticProperties (o)
getStaticProperties (o)
|> Array.tryFind (fun n -> n.Name = propName)
#endif
|> Array.tryFind (fun n -> n.Name = propName)

let trySetPropertyValue (o:obj) (propName:string) (value:obj) =
match tryGetPropertyInfo o propName with
match tryGetStaticPropertyInfo o propName with
| Some property when property.IsMutable ->
property.SetValue o value
true
| _ -> false

let tryGetPropertyValue (o:obj) (propName:string) =
try
match tryGetPropertyInfo o propName with
match tryGetStaticPropertyInfo o propName with
| Some v -> Some (v.GetValue(o))
| None -> None
with
Expand All @@ -65,7 +65,7 @@ module ReflectionUtils =

let removeProperty (o:obj) (propName:string) =

match tryGetPropertyInfo o propName with
match tryGetStaticPropertyInfo o propName with
| Some property when property.IsMutable ->
property.RemoveValue(o)
true
Expand Down
35 changes: 35 additions & 0 deletions tests/DynamicObject.Tests/DynamicObj.fs
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,44 @@ let tests_print = testList "Print" [
Expect.isTrue print "Print failed for issue 14"
]

let tests_copyDynamicProperties = testList "CopyDynamicProperties" [
testCase "NewObject" <| fun _ ->
let a = DynamicObj()
a.SetValue("a", 1)
a.SetValue("b", 2)
let b = a.CopyDynamicProperties()
Expect.equal a b "Values should be equal"
testCase "ExistingObject" <| fun _ ->
let a = DynamicObj()
a.SetValue("a", 1)
a.SetValue("b", 2)
let b = DynamicObj()
b.SetValue("c", 3)
a.CopyDynamicPropertiesTo(b)
Expect.equal (b.GetValue("a")) 1 "Value a should be copied"
Expect.equal (b.GetValue("b")) 2 "Value b should be copied"
Expect.equal (b.GetValue("c")) 3 "Value c should be unaffected"
testCase "NoOverwrite throws" <| fun _ ->
let a = DynamicObj()
a.SetValue("a", 1)
let b = DynamicObj()
b.SetValue("a", 3)
let f = fun () -> a.CopyDynamicPropertiesTo(b)
Expect.throws f "Should throw because property exists"
testCase "Overwrite" <| fun _ ->
let a = DynamicObj()
a.SetValue("a", 1)
let b = DynamicObj()
b.SetValue("a", 3)
Expect.notEqual a b "Values should not be equal before copying"
a.CopyDynamicPropertiesTo(b, true)
Expect.equal a b "Values should be equal"
]

let main = testList "DynamicObj" [
tests_set
tests_remove
tests_formatString
tests_combine
tests_copyDynamicProperties
]
6 changes: 3 additions & 3 deletions tests/DynamicObject.Tests/ReflectionUtils.fs
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,23 @@ let tests_PropertyHelper = testList "PropertyHelper" [

testCase "TryGetPropertyInfo" <| fun _ ->
let p = TestObject("1", "test")
let idOption = ReflectionUtils.tryGetPropertyInfo p "Id"
let idOption = ReflectionUtils.tryGetStaticPropertyInfo p "Id"
let id = Expect.wantSome idOption "Should have immutable property"
Expect.equal id.Name "Id" "Should have correct property"
Expect.equal id.IsStatic true "Id should be static"
Expect.equal id.IsDynamic false "Id should not be dynamic"
Expect.equal id.IsMutable false "Id should not be mutable"
Expect.equal id.IsImmutable true "Id should be immutable"

let nameOption = ReflectionUtils.tryGetPropertyInfo p "Name"
let nameOption = ReflectionUtils.tryGetStaticPropertyInfo p "Name"
let name = Expect.wantSome nameOption "Should have mutable property"
Expect.equal name.Name "Name" "Should have correct property"
Expect.equal name.IsStatic true "Name should be static"
Expect.equal name.IsDynamic false "Name should not be dynamic"
Expect.equal name.IsMutable true "Name should be mutable"
Expect.equal name.IsImmutable false "Name should not be immutable"

let nonExistingOption = ReflectionUtils.tryGetPropertyInfo p "NonExisting"
let nonExistingOption = ReflectionUtils.tryGetStaticPropertyInfo p "NonExisting"
Expect.isNone nonExistingOption "Should not have property"
]

Expand Down
Loading