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

Add hidden field #5

Merged
merged 9 commits into from
Oct 14, 2020
Merged

Add hidden field #5

merged 9 commits into from
Oct 14, 2020

Conversation

CristhianMotoche
Copy link
Contributor

Solves #2

Changes

  • Add Hidded, which is a new constructor for KeyValue
  • Ignoring any Hidden attribute by returning (VNull, undefined)
  • undefined won't be evaluated because its filtered from the KeyValue list

Please, let me know if this would solve the issue. 😃

@moleike moleike self-assigned this Oct 10, 2020
@moleike moleike linked an issue Oct 10, 2020 that may be closed by this pull request
@moleike
Copy link
Owner

moleike commented Oct 10, 2020

Hi Cristhian, thanks for your patch! I am reviewing this later 👍

Copy link
Owner

@moleike moleike left a comment

Choose a reason for hiding this comment

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

Very impressive work!
Please see my comments in the code. Also:

  • You might change the evalObj method to account for hidden fields, then in the VObj constructor change the keys from Text to a new type, e.g. data Key = Visible Text | Hidden Text the idea here is that fields should all be usable, the only time we care about whether it's a hidden field is when:
    • materializing an object (Value -> JSON), where hidden fields are filtered out
    • checking equality, again hidden fields are just ignored.

Also, keep in mind this is unrelated to private/public fields.

If this is not clear, please let me know!

@@ -221,7 +224,7 @@ arrayP = Fix <$> annotateLoc array
objectP :: Parser Expr'
objectP = Fix <$> annotateLoc object
where
object = mkObjectF <$> braces ((try methodP <|> pairP) `sepBy` comma)
object = mkObjectF <$> braces ((try hidden <|> try methodP <|> pairP) `sepBy` comma)
Copy link
Owner

@moleike moleike Oct 10, 2020

Choose a reason for hiding this comment

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

Methods can be hidden too, but here it would fail to parse { foo :: function(x) x }.
Here I would suggest changing the result to KeyValue key value hidden, where hidden is derived from paring either symbol : or ::.

@@ -65,7 +65,7 @@ instance Alpha UnyOp
data KeyValue a = KeyValue
{ key :: a,
value :: a
}
} | Hidden { key :: a }
Copy link
Owner

@moleike moleike Oct 10, 2020

Choose a reason for hiding this comment

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

Hidden fields, also have values, e.g.

{ foo : "bar", baz(x) :: x + 1 }

In this case, you might use the baz method in the program, regardless of the object being serialized to JSON.

As an example, the functions in the standard library are all hidden fields of the std object (https://jsonnet.org/ref/stdlib.html)

Copy link
Owner

@moleike moleike Oct 10, 2020

Choose a reason for hiding this comment

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

My suggestion here would be to add an additional field hidden :: Bool to the KeyValue constructor.

@CristhianMotoche
Copy link
Contributor Author

Thanks for the suggestions @moleike I'll review them later today.

@CristhianMotoche
Copy link
Contributor Author

CristhianMotoche commented Oct 13, 2020

Hi @moleike I tried your approach but I got stuck with this type error:

...l/jsonnet-haskell/src/Language/Jsonnet/Std.hs:42:26: error:
     No instance for (Integral Key) arising from a use of inj
     In the expression: inj (H.keys @Key @Value)
      In the expression: ("objectFields", inj (H.keys @Key @Value))
      In the expression:
        [("assertEqual", inj assertEqual), ("type", inj valueType),
         ("isString", inj (isType "string")),
         ("isBoolean", inj (isType "boolean")), ....]
   |
42 |         ("objectFields", inj (H.keys @Key @Value)),

Please, let me know if I'm going in the right direction or I lost at some point. I understood that manifest makes the final conversion of Value to JSON and it's used in toString and equalTo. Therefore, I should filter the visible keys there.

@CristhianMotoche
Copy link
Contributor Author

I managed to fix the error type, but I'm not sure about my implementation.

@moleike
Copy link
Owner

moleike commented Oct 13, 2020

Hi @CristhianMotoche, the problem you encounter is due to the impl. of objectFields method. So you don't need an HasValue instance for Key:

diff --git a/src/Language/Jsonnet/Eval.hs b/src/Language/Jsonnet/Eval.hs
index 8dc0428..d0c0993 100644
--- a/src/Language/Jsonnet/Eval.hs
+++ b/src/Language/Jsonnet/Eval.hs
@@ -124,10 +124,6 @@ instance HasValue Double where
   proj v = throwTypeMismatch "number" v
   inj = VNum
 
-instance HasValue Key where
-  proj = undefined
-  inj = undefined
-
 instance {-# OVERLAPS #-} Integral a => HasValue a where
   proj (VNum n) = pure (round n)
   proj v = throwTypeMismatch "number" v
diff --git a/src/Language/Jsonnet/Std.hs b/src/Language/Jsonnet/Std.hs
index ba2a1d3..2f8d67a 100644
--- a/src/Language/Jsonnet/Std.hs
+++ b/src/Language/Jsonnet/Std.hs
@@ -9,6 +9,7 @@ import Control.Applicative
 import Control.Monad.Except
 import qualified Data.ByteString as B
 import Data.Foldable
+import Data.HashMap.Lazy (HashMap)
 import qualified Data.HashMap.Lazy as H
 import Data.List
 import Data.Text (Text)
@@ -39,7 +40,7 @@ std = VObj $ (Thunk . pure) <$> H.fromList (map (\(k, v) -> (Visible k, v)) xs)
         ("isObject", inj (isType "object")),
         ("isArray", inj (isType "array")),
         ("isFunction", inj (isType "function")),
-        ("objectFields", inj (H.keys @Key @Value)),
+        ("objectFields", inj objectFields),
         ("length", inj length'),
         ("abs", inj (abs @Double)),
         ("sign", inj (signum @Double)), -- incl. 0.0, (-0.0), and NaN
@@ -98,6 +99,10 @@ std = VObj $ (Thunk . pure) <$> H.fromList (map (\(k, v) -> (Visible k, v)) xs)
         ("reverse", inj (reverse @Value))
       ]
 
+
+objectFields :: HashMap Key Value -> [Text]
+objectFields o = [k | Visible k <- H.keys o]
+
 assertEqual :: Value -> Value -> Eval Bool
 assertEqual a b = do
   a' <- manifest a

@moleike
Copy link
Owner

moleike commented Oct 13, 2020

I understood that manifest makes the final conversion of Value to JSON and it's used in toString and equalTo. Therefore, I should filter the visible keys there.

That's correct.

Please, let me know if I'm going in the right direction

Yes you are 👍

@CristhianMotoche
Copy link
Contributor Author

Thank you so much for you kind guidance @moleike Let me know if you have any other comment/suggestion. 😃

@moleike
Copy link
Owner

moleike commented Oct 14, 2020

@CristhianMotoche I think the issue is solved with your changes. Congrats on tackling this issue!

Copy link
Owner

@moleike moleike left a comment

Choose a reason for hiding this comment

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

LGTM

@moleike moleike merged commit 0bc5e7f into moleike:master Oct 14, 2020
@moleike
Copy link
Owner

moleike commented Oct 14, 2020

There's one minor issue remaining, but it can be fixed later on:
The parses will fail to parse { foo(x) :: x }, which is sugar for { foo :: function(x) x }.

@CristhianMotoche CristhianMotoche deleted the add-hidden branch October 15, 2020 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hidden fields
2 participants