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 all 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
12 changes: 0 additions & 12 deletions .github/workflows/build-and-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,6 @@ jobs:
python-version: '3.12'
- name: Setup Virtual Environment
run: python -m venv .venv
- name: Setup Poetry Windows
if: matrix.os == 'windows-latest'
run: |
.\.venv\Scripts\python.exe -m pip install -U pip setuptools
.\.venv\Scripts\python.exe -m pip install poetry
.\.venv\Scripts\python.exe -m poetry install --no-root
- name: Setup Poetry Unix
if: matrix.os == 'ubuntu-latest'
run: |
./.venv/bin/python -m pip install -U pip setuptools
./.venv/bin/python -m pip install poetry
./.venv/bin/python -m poetry install --no-root

# BUILD
- name: make script executable
Expand Down
6 changes: 0 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,4 @@ On windows you can use the `setup.cmd` to run the following steps automatically!

`py -m venv .venv`

4. Install [Poetry](https://python-poetry.org/) and dependencies

1. `.\.venv\Scripts\python.exe -m pip install -U pip setuptools`
2. `.\.venv\Scripts\python.exe -m pip install poetry`
3. `.\.venv\Scripts\python.exe -m poetry install --no-root`

Verify correct setup with `./build.cmd runtests` ✨
174 changes: 0 additions & 174 deletions poetry.lock

This file was deleted.

19 changes: 0 additions & 19 deletions pyproject.toml

This file was deleted.

97 changes: 59 additions & 38 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 this.TryGetStaticPropertyInfo name with
| 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 @@ -106,36 +133,30 @@ type DynamicObj() =
/// Returns both instance and dynamic properties when passed true, only dynamic properties otherwise.
/// Properties are returned as a key value pair of the member names and the boxed values
member this.GetProperties includeInstanceProperties : seq<KeyValuePair<string,obj>> =
#if FABLE_COMPILER_JAVASCRIPT || FABLE_COMPILER_TYPESCRIPT
FableJS.getPropertyHelpers this
|> Seq.choose (fun pd ->
if includeInstanceProperties || pd.IsDynamic then
new KeyValuePair<string, obj>(pd.Name, pd.GetValue this)
|> Some
this.GetPropertyHelpers(includeInstanceProperties)
|> Seq.choose (fun kv ->
if kv.Name <> "properties" then
Some (KeyValuePair(kv.Name, kv.GetValue this))
else
None
None
)
#endif
#if FABLE_COMPILER_PYTHON
FablePy.getPropertyHelpers this
|> Seq.choose (fun pd ->
if includeInstanceProperties || pd.IsDynamic then
new KeyValuePair<string, obj>(pd.Name, pd.GetValue this)
|> Some
else
None

/// 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)
)
#endif
#if !FABLE_COMPILER
seq [
if includeInstanceProperties then
for prop in ReflectionUtils.getStaticProperties (this) ->
new KeyValuePair<string, obj>(prop.Name, prop.GetValue(this))
for key in properties.Keys ->
new KeyValuePair<string, obj>(key, properties.[key]);
]
#endif
|> Seq.filter (fun kv -> kv.Key.ToLower() <> "properties")

/// 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)
Expand Down
Loading
Loading