Skip to content

Conversation

sten0
Copy link
Contributor

@sten0 sten0 commented Apr 10, 2020

Post-edits, this docstring makes it sound like this toggle enables a
kill-ring or clipboard history for the Python shell, which sounds like
it's always a good thing. That said, it's disabled by default. I
believe it would be useful to let the user know why it's disabled by
default.

Also, I think it would be useful to have a follow up commit that
explains how it "affects the following functions:…".

PR checklist

Please make sure that the following things have been addressed (and check the relevant checkboxes):

  • Commits respect our guidelines
  • Tests are passing properly (see here on how to run Elpy's tests)

For new features only:

  • Tests has been added to cover the change
  • The documentation has been updated

Post-edits, this docstring makes it sound like this toggle enables a
kill-ring or clipboard history for the Python shell, which sounds like
it's always a good thing.  That said, it's disabled by default.  I
believe it would be useful to let the user know why it's disabled by
default.

Also, I think it would be useful to have a follow up commit that
explains how it "affects the following functions:…".
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.145% when pulling 14e46e7 on sten0:1.33.0-spelling-fixes into 7d0f458 on jorgenschaefer:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.145% when pulling 14e46e7 on sten0:1.33.0-spelling-fixes into 7d0f458 on jorgenschaefer:master.

@gopar
Copy link
Collaborator

gopar commented Apr 22, 2020

Has there been other ppl confused/misunderstood about that docstring?

Otherwise LGTM.

@sten0
Copy link
Contributor Author

sten0 commented Apr 28, 2020

Hi @gopar, thank you for reviewing! By the way, do you know why elpy-shell-add-to-shell-history is disabled by default? If you or @galaunay aren't planning to document that in a follow-up commit, then I'd like to include it in the PR for completeness sake. If necessary I'd also be happy to investigate how it changes the behaviour of the listed functions, and document this for the same reason.

@gopar
Copy link
Collaborator

gopar commented Apr 30, 2020

@sten0 I'm not sure why it's disabled by default. Must of been a decision from a while ago.

Yeah, if you can document that extra bit, that would be awesome :)

Ping me whenever it's ready. No rush. Thanks! \o/

@galaunay
Copy link
Collaborator

galaunay commented May 2, 2020

This was disabled by default because we suspected it could be annoying (e.g. when you send big chunks of code that you don't want to find back in your history).
That's also why it only affects some functions (send-defun, send-defclass and send-buffer were set aside).

I agree the docstring should at least mention that.
Or if you have a another solution it could be the opportunity to make it better ?

@gopar
Copy link
Collaborator

gopar commented Feb 27, 2021

@sten0 let me know if you wanted to do that follow up doc change. otherwise i'll commit this.

@galaunay galaunay force-pushed the master branch 4 times, most recently from c4a2564 to d974e00 Compare June 30, 2021 22:40
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.

4 participants