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

[#126] Add mount integration #127

Merged
merged 2 commits into from
Feb 22, 2020

Conversation

bn-darindouglass-zz
Copy link

  • Add new formatting-stack.mount namespace for configuring on-up
    hooks
  • Add mount tests and example repl/dev implementation

Brief

#126

QA plan

Green tests

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
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, 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 reviewed the PR
  • Additionally, 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)

- Add new `formatting-stack.mount` namespace for configuring `on-up`
  hooks
- Add mount tests and example repl/dev implementation
@bn-darindouglass-zz
Copy link
Author

I'm not seeing a build spin up, so I haven't checked that box. However, tests do pass locally.

@vemv
Copy link
Contributor

vemv commented Feb 21, 2020

I'm not seeing a build spin up, so I haven't checked that box. However, tests do pass locally.

👍! I appreciate the care being placed.

Sometime soon I'll merge this directly (to a branch other than master), so that I can apply suggestions directly. That way we can cut a release quickly including your PR.

Thanks for the contribution!

@bn-darindouglass-zz
Copy link
Author

My pleasure! I use mount basically exclusively and didn't want to go about hacking mount + formatting-stack locally. :)

:in-background? false
:reporter (reporters.passthrough/new)}]
(sut/configure opts)
(mount/start)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would help me to understand - can (mount/start) perform code-loading (like a clojure.core/require or a tools.namespace.repl/refresh)?

If that were the case, this test would be unlike its Component and Integrant cousins

Copy link
Author

@bn-darindouglass-zz bn-darindouglass-zz Feb 21, 2020

Choose a reason for hiding this comment

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

mount/start isn't really smart in-and-of-itself, it really just calls :start on all the defstates that it knows about.

However, mount automatically handles re-compilation (basically just (mount/stop) (mount/start)) of its defstates whenever a namespace is recompiled.

So if someone ran (require ... :reload) or ran C-c C-k in a CIDER repl on a namespace with a defstate, all hooks would be called.

@vemv vemv changed the base branch from master to 126--mount February 22, 2020 01:55
@vemv
Copy link
Contributor

vemv commented Feb 22, 2020

Merging against nedap:126--mount where I'll apply feedback directly

@vemv vemv merged commit 6a5a1c7 into nedap:126--mount Feb 22, 2020
@vemv vemv mentioned this pull request Feb 22, 2020
28 tasks
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