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.

Sign up for GitHub

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

Jump to bottom

Extended coverage #77

Merged
merged 3 commits into from
Jul 26, 2019
Merged

Extended coverage #77

merged 3 commits into from
Jul 26, 2019

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Jul 26, 2019 •
edited
Loading

Brief

Part of #57

  • Extend coverage
  • Fix JVM variant of instance-spec to also consider protocols
    • Already happened in the cljs variant

QA plan

Green build

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • I have self-reviewed the PR prior to assignment, and its build passes
  • Specifically, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Modular design
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Business-facing correctness
    • Robustness (red paths, failure handling etc)
    • Modular design
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

vemv added 3 commits July 26, 2019 09:34
@vemv
Cover consistent-tagging?
6ef2552
@vemv
nedap.speced.def.impl.parsing/instance-spec: also span protocols, f…
216b4fa 
…or both compilation targets

Prior to this commit, only cljs was being covered.
@vemv
extract-specs-from-metadata: avoid emitting spurious type annotations
2600999 
In practice these were never seen (since later functions would fix them), but it was undesirable to have this pipeline of bad output -> amended output.

Also the tests had to reflect this bad output (now fixed).
@vemv vemv marked this pull request as ready for review July 26, 2019 10:08
@vemv
Copy link
Contributor Author

vemv commented Jul 26, 2019

Merging as it's been self-reviewed and it is straightforward stuff.

@vemv vemv merged commit f4d3b7f into master Jul 26, 2019
@vemv vemv deleted the 57 branch July 26, 2019 10:09
@vemv vemv mentioned this pull request Jul 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vemv