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

Added name to data spec #125

Closed

Conversation

furkan3ayraktar
Copy link

Fixes #124

Adds name to data spec when a name is provided during creation.

Example:

(def my-data
  (ds/spec {:name :model/my-data
            :spec {:id    common/uuid-string?
                   :name  string?
                   :age   pos-int?}}))

(st/spec-name my-data)
;; => :model/my-data

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 97.447% when pulling 691e994 on furkan3ayraktar:add-name-to-data-specs into cb4b448 on metosin:master.

@RokLenarcic
Copy link

Is this planned for some release of sorts? data-spec not naming the specs it generates creates a very unfortunate result:


-- Spec failed --------------------

  {:personalTemplates [{:templateTypeEnum "ORG"}]}
                       ^^^^^^^^^^^^^^^^^^^^^^^^^

should contain keys: :linkId, :name

|     key | spec |
|---------+------|
| :linkId |  nil |
|   :name |  nil |

-------------------------
Detected 1 error

As you can see, expound names specs in error as nil.

@ikitommi
Copy link
Member

Thanks for the PR and sorry for the lag, got buried after vacations. Would like to merge this, but the tests don't pass. Could you fix them?

@RokLenarcic
Copy link

I'll try to solve it with fewer changed lines if possible, we'll see.

@furkan3ayraktar
Copy link
Author

furkan3ayraktar commented Sep 18, 2018

I also realized my solution might not be the best possible way of doing it. I couldn't have time to think thoroughly.

@ikitommi
Copy link
Member

I think it could be enough to put the name into response of -map-spec, e.g. here https://github.com/metosin/spec-tools/blob/master/src/spec_tools/data_spec.cljc#L212

@ikitommi
Copy link
Member

It seems that just assoc'in the :name does it. We can't do it for non-maps as they would have the same name, e.g. (ds/spec ::kikka [[int?]]) would have two ::kikka specs.

#132

@ikitommi
Copy link
Member

If you would like to copy the #132 changes into this PR, would be happy to merge this and remove my test branch. Or, I can do it other way around.

@furkan3ayraktar
Copy link
Author

furkan3ayraktar commented Oct 1, 2018

I didn't understand why we can't do this to non-map specs. If I'm able to assoc a name after I create the spec, it should also be possible to add it anyway within the spec-tools, or maybe I'm missing something.

(def my-spec
  (assoc
    (ds/spec {:name ::my-spec
              :spec [[int?]]})
    :name ::my-spec))

(st/spec-name my-spec)
;; => core/my-spec

@ikitommi
Copy link
Member

ikitommi commented Oct 2, 2018

You're correct, all top-level specs of any type and direct map value specs should have a name. But specs wrapped in internal collections can't have a name as they can't have unique names as they don't accumulates anything to the name.

example1

(ds/spec :user/kikka [[int?]])
  • the top-level coll-spec has name :user/kikka
  • the nested coll-spec is anonymous
  • the int? is anonymous

example2

(ds/spec ::kikka [{:a [[{:b int?]]}])
  • the top-level coll-spec has name :user/kikka
  • the nested map is anonymous
  • the coll-of under :a has name :user$kikka/a
  • the nested coll-of under previous
  • thje nested map under previous is anonymous
  • the int? under:b has name :user$kikka.a/b

... hopefully got that correct. in brief: just don't push the name into a spec if the name is already been given to a spec under the data-spec, meaning non-top-level collections of any kind.

I think.

@ikitommi
Copy link
Member

FIxed this with #140

@ikitommi ikitommi closed this Oct 21, 2018
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