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

Advising System #107

Merged
merged 14 commits into from
Sep 12, 2021
Merged

Advising System #107

merged 14 commits into from
Sep 12, 2021

Conversation

jaidetree
Copy link
Collaborator

@jaidetree jaidetree commented Aug 28, 2021

Provides a system to wrap or define advisable functions that can be overwritten later. See #72 for intended use case.

Can solve a number of open PRs by allowing us to wrap some of the internals to support advising. Then users can customize the behaviors by adding advice to their liking. This helps us reduce the number of config settings we need to include for personal preferences.

For example:

We can make the main modal.fnl alert function advisable, then users can add advice to override it so that it displays on all monitors at the same time.

The work here depends on the testing work in #106.

Usage example:

(import-macros {: defn
                : defadvice} :lib.advice.macros)

(defn original-fn
      [x y z]
      "docstr"
      "Hi")

(original-fn)
;; => "Hi"

(defadvice advice-fn
           [x y z]
           :override original-fn
           "Overrides original-fn"
           "over-it")

(original-fn)
;; => "over-it"

Closes #101
Closes #72

Test Results and feature list:

image

Credits

Big thanks to @Grazfather for getting a first draft out I could iterate on. We also paired a few weeks back and traded knowledge around this which helped a lot.

@jaidetree jaidetree force-pushed the feature/advice branch 2 times, most recently from c11ab1b to ba49b83 Compare August 29, 2021 01:22
@jaidetree
Copy link
Collaborator Author

Added a couple extra APIs for tracking advice, among refining the implementation, as well as completing the documentation.

Latest draft may be found https://github.com/agzam/spacehammer/blob/d100cb5368f20679cd48699b73333ab599440b34/docs/advice.org.

This was referenced Aug 29, 2021
@jaidetree
Copy link
Collaborator Author

Received some feedback from Grazfather, and ambridsall in the doom emacs discord. Updated the advice docs and caught a mismatch in the after behavior https://github.com/agzam/spacehammer/blob/6fb8e1c5d678c4d6d5641a55a6748dcc2ac1395d/docs/advice.org.

@jaidetree
Copy link
Collaborator Author

@jaidetree
Copy link
Collaborator Author

Unit testing was merged so this is ready for review whenever you're available @Grazfather

Grazfather
Grazfather previously approved these changes Sep 12, 2021
Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Saw 2 rendering issues, LGTM overall.

core.fnl Outdated Show resolved Hide resolved
docs/advice.org Outdated Show resolved Hide resolved
docs/advice.org Outdated Show resolved Hide resolved
lib/advice/init.fnl Show resolved Hide resolved
Grazfather
Grazfather previously approved these changes Sep 12, 2021
@jaidetree jaidetree merged commit 0825723 into agzam:master Sep 12, 2021
jaidetree added a commit to jaidetree/spacehammer that referenced this pull request Sep 12, 2021
agzam#107

Added an advice doc, still WIP though

Fixed some small types in docs/advice.org

Fixed small formatting issues in docs/advice.org

Finished advice docs and refined implementation

Documented one last consideration in advice.org

Updated advice.org section order

Thanks to ambirdsall from doom discord for the suggestion

Reorder advising doc for users

Update advice docs, fix after advise type return value

Updated advice org formatting

More advice.org formatting updates from discord feedback

Optimize advice entry lookup on every dispatch call

Fixed PR review issues for advice

Fixed advice.org formatting
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.

Feature request: advising + defadvice implementation
2 participants