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

3944 extend the properties api to better support nested configuration #3952

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jan 13, 2024

The implementation closely aligns with the original design, extensively incorporating existing code to minimize workload costs. The new API maintains a consistent style with the old API, which remains unchanged.
Features
With new expose stuff:
KeyNamePath -- path to search for properties
definePropertiesProperty -- define nested property
usePropertyByPath -- extract property by path
usePropertyByPathEither -- same as above
usePropertyByPathAction -- action api for usePropertyByPath
HasPropertyByPath -- constraint for using usePropertyByPath like the HasProperty

We can now define properties upon properties to create nested one. And use KeyNamePath to retrieve the property

    nestedPropertiesExample = emptyProperties
        & definePropertiesProperty #parent "parent" (emptyProperties & defineStringProperty #foo "foo" "foo")
        & defineStringProperty #baz "baz" "baz"

    nestedPropertiesExample2 = emptyProperties
        & definePropertiesProperty #parent "parent" (emptyProperties & defineStringProperty #foo "foo" "xxx")
        & defineStringProperty #baz "baz" "baz"

    examplePath1 = SingleKey #baz
    examplePath2 = ConsKeysPath #parent (SingleKey #foo)

To retrieve we can have

usePropertyByPathEither examplePath2 nestedPropertiesExample object

Todo list:

  • add key name path
  • add usePropertyByPathEither, usePropertyByPath to cope with usage of key name path
  • add more test
  • add action to use the above

This should close #3944, old api would remains the same.

@soulomoon soulomoon marked this pull request as ready for review January 13, 2024 23:37
@soulomoon soulomoon enabled auto-merge (squash) January 14, 2024 16:54
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 14, 2024

I think this is done. It provide serveral new functions to deal with nested properties(new functions and the use example at the first comment ). Also I've added test for this new api as prof of correctness.
Can you guys take a look if this looking good? @michaelpj @fendor

@soulomoon soulomoon enabled auto-merge (squash) January 14, 2024 18:36
@fendor fendor added the status: needs review This PR is ready for review label Apr 17, 2024
@fendor fendor self-assigned this Apr 17, 2024
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

First, sorry for taking forever to get this reviewed! It is rather difficult code to review :)
Second, thanks for taking care of the Properties module!

I mostly have some small nitpicks, especially regarding the formatting. Could you make sure to add a newline between instance declarations and normalise the formatting a little bit? E.g. ,HasProperty vs , ParsePropertyPath.

Also, a little bit more documentation in the code would be nice, to explain what problem TProperties is supposed to solve.

Otherwise, looks good to go to me!

hls-plugin-api/src/Ide/Plugin/Properties.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/Plugin/Properties.hs Outdated Show resolved Hide resolved
@@ -57,6 +66,7 @@ data PropertyType
| TObject Type
| TArray Type
| TEnum Type
| TProperties [PropertyKey]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't map naturally the JSON type any more, could you add a comment why we want/need this?

hls-plugin-api/src/Ide/Plugin/Properties.hs Outdated Show resolved Hide resolved
Comment on lines 155 to 159
propertyTest = testGroup "property api tests" [
testCase "property toVSCodeExtensionSchema" $ do
let expect = "[(\"top.baz\",Object (fromList [(\"default\",String \"baz\"),(\"markdownDescription\",String \"baz\"),(\"scope\",String \"resource\"),(\"type\",String \"string\")])),(\"top.parent.foo\",Object (fromList [(\"default\",String \"foo\"),(\"markdownDescription\",String \"foo\"),(\"scope\",String \"resource\"),(\"type\",String \"string\")]))]"
let result = toVSCodeExtensionSchema "top." nestedPropertiesExample
show result @?= expect
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we perhaps turn tests like these into golden tests? It is difficult to review what's going on here.

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 16, 2024

Thanks for the review @fendor. This indeed is hard to review with all the type level fiddling. I've adjusted the test and add a comment, see if it is better?

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fendor fendor requested a review from wz1000 as a code owner May 18, 2024 11:42
@soulomoon soulomoon merged commit b43dcbb into master May 18, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the Properties api to better support nested configuration
2 participants