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

#merge #ref fails under some circumstances. #85

Closed
mchughs opened this issue Jan 24, 2020 · 8 comments
Closed

#merge #ref fails under some circumstances. #85

mchughs opened this issue Jan 24, 2020 · 8 comments

Comments

@mchughs
Copy link

mchughs commented Jan 24, 2020

My convoluted configuration looks something like

{ ...
:bar #profile {:some-profile {:value #or [#ref [:some-path #keyword #env MY_ENV]
                                          "my-backup-value"]}}
:foo #merge [{:w :x :y :z}
             #ref [:bar]]
...}

When I apply aero's reader on this .edn I'll get something like

{ ...
:bar {:value "my-backup-value"}
:foo {:w :x :y :z}
...}

The merge bit fails. However if I alter some other piece of the map such as duplicating a key-value pair such as

{ ...
:bar #profile {:some-profile {:value #or [#ref [:some-path #keyword #env MY_ENV]
                                          "my-backup-value"]}}
:foo #merge [{:w :x :y :z}
             #ref [:bar]]
:foo-again #merge [{:w :x :y :z}
                    #ref [:bar]]
...}

Then it will properly evaluate to

{ ...
:bar {:value "my-backup-value"}
:foo {:w :x :y :z :value "my-backup-value"}
:foo-again {:w :x :y :z :value "my-backup-value"}
...}

This makes me suspect that some evaluation-order issue is responsible. Looking at the implementation of the #merge reader I don't see any piece for checking for incomplete contents so I'm surprised it seems to work so often.

Unfortunately I could not recreate a small example, and even anonymizing the keys and values in my personal config destroy the behavior so I cannot share that either.

@SevereOverfl0w
Copy link
Contributor

Merge is a "function" not a macro, so it isn't supposed to be concerned with incomplete items.

@SevereOverfl0w
Copy link
Contributor

On mobile, bug is in

(defmethod eval-tagged-literal :default

[tl opts env ks]

(let [{:keys [:aero.core/incomplete?] :as expansion}

(expand (:form tl) opts env ks)]

(if incomplete?

(update expansion ::value (rewrap tl))

(update expansion ::value #(reader opts (:tag tl) %)))))

@SevereOverfl0w
Copy link
Contributor

It should probably expand repeatedly in the same way #or does

@SevereOverfl0w
Copy link
Contributor

I am having trouble reproducing this. My suspicion was that the arguments to #merge weren't being expanded recursively first. As far as I can tell they are though. That leaves me a little bit stumped.

@SevereOverfl0w
Copy link
Contributor

Okay, finally giving up. If you redefine aero.alpha.core/expand as:

(def level (atom 0))

(defn expand
  "Expand value x.  Dispatches on whether it's a scalar or collection.  If it's
  a collection it will expand the elements of the collection."
  [x opts env ks]
  (let [lvl (swap! level inc)]
    (print (apply str "|" (repeat lvl \space)) " <= ")
    (pr x)
    (print " ")
    (prn ks)
    (let [res (if (or (and (map? x) (not (record? x))) (set? x) (seq? x) (vector? x))
                (expand-coll x opts env ks)
                (expand-scalar x opts env ks))]
      (print (apply str "|" (repeat lvl \space))  " => ")
      (prn (select-keys res [:aero.core/incomplete? :aero.core/incomplete]))
      (reset! level (dec lvl))
      res)))

It will print out some useful debugging information about the order of things tried, when/if they were marked incomplete, and what went in. tools.trace was producing way too much output because of how big env gets. The code is a little messy.

Hopefully this will provide you the data to figure out a minimal repro

@mchughs
Copy link
Author

mchughs commented Jan 25, 2020

Awesome. I will get back to you with my findings.

@mchughs
Copy link
Author

mchughs commented Jan 30, 2020

I think I would like to close the issue because in simplifying down the config.edn this issue has been avoided. I don't think there are many actionable steps for this issue given I couldn't recreate the issue. Thoughts?

@SevereOverfl0w
Copy link
Contributor

Maybe the next person will have a clearer picture of what's going on (or maybe you weren't understanding your complex config properly? 😜).

Let's close until someone figures it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants