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

Introduce cider-log-dwim #3592

Closed
wants to merge 1 commit into from
Closed

Introduce cider-log-dwim #3592

wants to merge 1 commit into from

Conversation

vemv
Copy link
Member

@vemv vemv commented Dec 2, 2023

Following #3591, I was checking how to use CIDER Log in a more streamlined fashion.

My thinking is, in day-to-day usage, I should be able to run a single command that doesn't make me think about anything at all, and have the *cider-log* rendered.

I did get an appreciation for transient-mode, but ideally one would only need it for fine-grained manipulations.

So this PR:

  • Introduces defvar cider-log-preferred-framework nil
    • Necessary for that "don't make me think" experience 😄
  • Warns when Logview isn't installed
    • TODO: also warn when no consumer is associated, if not using cider-log-dwim
  • Introduces cider-log-dwim, meant as an idempotent command for showing the logs.

Leaving it as draft for now - UX tweaks welcome. There may be also room for improvents as for how I used the internals?

Cheers - V

@bbatsov
Copy link
Member

bbatsov commented Dec 6, 2023

I'm not a big fan of dwim commands, as often people understand those different and they are usually something that could be resolved at a different level (e.g. by tweaking the base functionality). Perhaps it'd be nice to start by outlining what exactly are the problems this is aiming to resolve? E.g. I think the preferred-log-framework can easily be integrated into the existing command to start the mode.

@vemv
Copy link
Member Author

vemv commented Dec 7, 2023

Perhaps it'd be nice to start by outlining what exactly are the problems this is aiming to resolve?

In my view, the default workflow represents an excess of steps for getting to see the logs.

...I like the book title Don't Make Me Think as a general UX guideline 😄

In specific terms:

  • I don't want to think about the concepts of producer and consumer every time
  • I won't want to see/use a transient UI for the "80%" use case
  • I want to immediately see a buffer with the logs, practically every time (even if I might perform some other log-related action afterwards)

So, the proposed cider-log-dwim accomplishes that.

I understand the concern about dwim naming. However, one can observe:

  • "show me the logs" is a pretty universal intent
    • people are used to tail -f / less +F and see logs immediately
  • We can write down the 'contract' for what dwim means here, in the docstring.

Ultimately, we could find an alternative name, but e.g. cider-log-setup-idempotently-and-show might not exactly help.

cider-log-show maybe? (the "setup idempotently" part would be implied)

@bbatsov
Copy link
Member

bbatsov commented Dec 7, 2023

My point was that we can probably enhance cider-log instead of introducing a different command or make it behave differently with a prefix arg (e.g. it can try to infer some of the things that are explicitly set today). Fewer commands usually result in better UX.

@vemv
Copy link
Member Author

vemv commented Dec 7, 2023

cider-log currently isn't a vanilla function, but a transient-define-prefix thing. So one couldn't prevent the transient UI from popping up, even if it wasn't necessary for the "80% case".

Maybe we can rename transient-define-prefix cider-log to transient-define-prefix cider-log-transient, and move some of its logic to a new cider-log function which would do what cider-log-dwim does as of this PR.

But that would be a breaking change.

@bbatsov
Copy link
Member

bbatsov commented Dec 14, 2023

Okay, let's go with the cider-log-dwim naming then and document clearly the differences between both commands.

@vemv
Copy link
Member Author

vemv commented Dec 18, 2023

@r0man - last chance if you'd like to add any observation!

@r0man
Copy link
Contributor

r0man commented Dec 20, 2023

Hi,

I just tried cider-log-dwim, but I'm not sure if we actually need
another command for this. I believe we mostly have what it tries to
do. And maybe we can enhance/rename the existing commands.

To setup CIDER log mode, you don't necessarily have to go through the
cider-log command. Here is what you could do instead:

  1. Run cider-jack-in in the cider-nrepl repository.

  2. Run cider-log-event. This will ask you for the log framework,
    will add the log appender, to capture events, and switch to the
    *cider-log* buffer.

  3. Open the cider.nrepl.middleware.log-tests namespace and run the
    tests (which do log some events).

  4. Take a look at the *cider-log* buffer and you will see the events.

Those are the minimum steps needed to do. Step 1 and 4 are always
needed, so I think we can optimize 2 and 3.

I have a couple of suggestions:

  • If you have cider-log-framework-name customized, you won't get
    asked for the framework in step 2. So maybe we should better
    document this. If you have exactly 1 framework then I believe you
    won't get asked at all. But this is a rare case, since on a JDK you
    always have the JUL framework (which nobody uses).

  • Maybe the cider-log transient is too much for a new user. We could
    rename cider-log to cider-log-menu, and make cider-log-event
    the main entry point by renaming it to cider-log.

  • I could imagine another issue for confusion is, that you have to
    attach a log appender before you actually see events appearing in
    the buffer. We could automatically add the appender if
    cider-log-framework-name is customized after jack in. But I would
    add this behind a feature flag. I think it really depends when you
    want logs to be captured. For example if you have a project that
    uses generative testing a lot, and some code paths are logging, you
    don't want this enabled by default.

  • Maybe add a quick-start section to the documentation that shows the
    steps needed as a numbered list, by showing how to set it up with
    the JUL framework.

Wdyt?

@r0man
Copy link
Contributor

r0man commented Dec 20, 2023

@vemv I believe you can prevent the transient menu from popping up. The popping up is just the default behaviour if the transient does not have a body defined. I think the popup happens in transient-setup.

So, if you don't call transient-setup in the following line, I think it won't pop up and you could do something else instead:
https://github.com/r0man/cider/blob/25c51fd4b1246801ba3fa15c57b44d860772417c/cider-log.el#L1423

@bbatsov
Copy link
Member

bbatsov commented Dec 22, 2023

Great suggestions by @r0man - seems we really need to expands the docs a bit. Also:

Maybe the cider-log transient is too much for a new user. We could
rename cider-log to cider-log-menu, and make cider-log-event
the main entry point by renaming it to cider-log.

I'd be fine with this suggestion, although perhaps we can come up with a better name than cider-log-menu.

@vemv
Copy link
Member Author

vemv commented Dec 29, 2023

Thanks a lot for the input, both!

I'm not sure about the breaking changes part. Perhaps documenting the two main approaches (programmatic and transient-driven) would suffice.

We could leave renaming for later if it turned out to be a frequent pain point?

I'll close this PR and will be checking out cider-log-event - might propose doc tweaks afterwards.

@vemv vemv closed this Dec 29, 2023
@vemv
Copy link
Member Author

vemv commented Dec 29, 2023

@r0man - I cider-log-event and while it works (honoring the defcustom and setting things up, as you mentioned), it's also transient-based.

It also doesn't show the cider-log buffer instantly (step 4) - personally I strongly feel it should show without user intervention.

I think I'll give another stab at this, keeping the feedback in mind.

@r0man
Copy link
Contributor

r0man commented Dec 31, 2023

Hi @vemv, maybe we can change cider-log-event in a way that it only shows the transient if the *cider-log-buffer* is already open (to refine options, or adjust search options) and otherwise open it directly?
Please feel free to experiment, whatever you think is most efficient. I have nothing against that.

@vemv
Copy link
Member Author

vemv commented Jan 1, 2024

Got it! Feel free to check out #3602

maybe we can change cider-log-event in a way that it only shows the transient if the cider-log-buffer is already open (to refine options, or adjust search options) and otherwise open it directly?
Please feel free to experiment, whatever you think is most efficient. I have nothing against that.

I don't know, it seems to me that it would be desirable to have predictable UXs: one (cider-log-show) that never uses transient-mode, and one (cider-log-event) that always does.

That would seem clearer to users and maintainers alike.

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