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

Fail when =contains=> targets a non-map value #399

Merged
merged 2 commits into from Aug 16, 2017

Conversation

philomates
Copy link
Collaborator

@philomates philomates commented Aug 3, 2017

Ran into a bug when using ..some-metaconstant.. =contains=> [1 2 3] only to realize that =contains=> was never meant to be used with lists, or non-map values for that matter.

For example, the current metaconstant merging logic, which runs when ..mc.. =contains=> ... appears twice in against-background/provided forms, doesn't work with non-map values.

Also, what would be the semantics of the following?:

(fact 
  (first (g)) => ????
  (provided 
    (g) => ..result..
    ..result.. =contains=> [1 2]
    ..result.. =contains=> [3 4]))

One could argue that letting =contains=> operate over sets would be useful and have clear semantics, but I'm in favor of restricting the capabilities of =contains=> because it is already a bit confusing to use.

The approach this PR takes to addressing the problem is to change the behavior of =contains=> from:

(use 'midje.repl)

(defn get-metaconstant-storage-str [m]
  (with-out-str (print (.storage m))))

(unfinished a-func)
(fact "example showing that you used to be able to store anything in a metaconstant"
  (get-metaconstant-storage-str (a-func)) => "[1]"
  (provided
    (a-func) => ..result.. 
    ..result.. =contains=> [1]))

to

(fact
  (get-metaconstant-storage-str (a-func)) => "[1]"
  (provided
    (a-func) => ..result.. 
    ..result.. =contains=> [1]))

FAIL at (form-init935422476386958100.clj:2)
    Midje could not understand something you wrote: 
        In `..result.. =contains=> [1]`, [1] is not a map.

@@ -1,4 +1,4 @@
(defproject midje "1.9.0-alpha8"
(defproject midje "1.9.0-alpha9"
Copy link
Collaborator Author

@philomates philomates Aug 3, 2017

Choose a reason for hiding this comment

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

Figure this is worth releasing because #390, released in alpha8, makes it more probable that users will hit the buggy logic in merge-metaconstants when trying to use =contains=> with lists.

Copy link

@viniciushana viniciushana left a comment

Choose a reason for hiding this comment

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

👏

@sovelten
Copy link

👍

@philomates philomates merged commit bf3bf82 into marick:master Aug 16, 2017
@philomates philomates deleted the pm/error-on-contains-list branch August 16, 2017 13:56
@MDIB
Copy link

MDIB commented Aug 16, 2017

:rubinho:, but 👏

@philomates
Copy link
Collaborator Author

@viniciushana & @EricVM looks like I missed the case where =contains=> points to a variable that points to a map or non-map value. Should be covered in this PR: #400

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

Successfully merging this pull request may close these issues.

None yet

4 participants