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

fixing missing titles with s/cat and s/merge #223

Merged
merged 1 commit into from
Mar 28, 2020

Conversation

wandersoncferreira
Copy link
Contributor

Hi! This PR attempt to solve #197 however there are some points I wanted to make clear.

  1. I might be obvious, but the :title will be show up only if your s/merge or s/cat is defined because
(st/spec-name (s/merge ::one ::two)) ;; => nil

Therefore, only if you use something like:

(s/def ::merged (s/merge ::one ::two))
(st/spec-name ::merged) ;; => full-qualified/merged
  1. This PR depends on another open PR allow option to toggle title inference from specs #221 which will enable the title added by this PR to be customizable by the end user. However, I am not sure how to deal with this dependency here. What I end up doing was to leave this PR completely independent from the other one and once both of them is merged I will open a new one finishing the work. I did this because the other PR is more opiniated and if we have implementations change it could affect this one too.

Thks

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 97.89% when pulling 800e354 on wandersoncferreira:fix/missing-titles into d043282 on metosin:master.

@ikitommi ikitommi merged commit 6d1ffd4 into metosin:master Mar 28, 2020
@ikitommi
Copy link
Member

Thanks again :)

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.

3 participants