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

Add justl-per-recipe-buffer custom variable #45

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

TristanCacqueray
Copy link
Contributor

Fixes #44

@TristanCacqueray
Copy link
Contributor Author

Running the tests with and without the custom seem to work, though I am not sure how to test the setting actually work, e.g. can we set the custom for the duration of a given test?

Also the :sentinel arg does not seem to be used and since this change the api I think we should remove it.

@psibi
Copy link
Owner

psibi commented Jan 24, 2024

@TristanCacqueray LGTM overall.

can we set the custom for the duration of a given test?

Not sure. But can you write a test such that you do customize-set-variable and then run a recipe and check that the new buffer is actually created ? And then at the end reset using customize-set-variable to the original value so that it doesn't affect other test.

Also, if you manually test to check that TRAMP integration is not broken using the following steps: https://github.com/psibi/justl.el/tree/master/test ? Alternatively, I can do that at the end before merging this.

One more thing: If you can update the changelog and README with an explanation of this defcustom, that will be helpful also.

Also the :sentinel arg does not seem to be used and since this change the api I think we should remove it.

As long as the test passes and the TRAMP integration doesn't break, go for it. :-)

@TristanCacqueray
Copy link
Contributor Author

@psibi Thanks for the prompt review and the suggestions. I have validated that this change works over TRAMP, but using ssh instead of docker (because my emacs is already running in a container :).

@psibi psibi merged commit 476bf9c into psibi:master Jan 24, 2024
4 checks passed
@psibi
Copy link
Owner

psibi commented Jan 24, 2024

Thank you!

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.

Create a buffer per command
2 participants