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 regexp prompt matching (+ style and testing) #82

Merged
merged 12 commits into from
Jun 17, 2020
Merged

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jun 12, 2020

Right, in preparation for #52, I've whipped this package into shape:

  1. Added pre-commit configuration and CI style testing
  2. Extracted the "DOM independent" JS code to a separate file, in order to
  3. Add CI testing for the JS code
  4. Added documentation on running the tests

@chrisjsewell
Copy link
Member Author

Note to self, you can also use https://github.com/jsdom/jsdom to mock the DOM

@chrisjsewell chrisjsewell changed the title Improve package code style and testing Add regexp prompt matching (+ code style and testing) Jun 13, 2020
@chrisjsewell chrisjsewell changed the title Add regexp prompt matching (+ code style and testing) Add regexp prompt matching (+ style and testing) Jun 13, 2020
@chrisjsewell
Copy link
Member Author

Ok I've gone ahead and implemented the regexp matching, see: https://143-140029949-gh.circle-artifacts.com/0/html/index.html#using-regexp-prompt-identifiers

The one thing that I won't add in this PR, but should be considered for completeness is regexes per selector, e.g. if you have

$ cd path

and

>>> print("$ cd path")
$ cd path

You only want the first one to copy the $ line

@championshuttler
Copy link

championshuttler commented Jun 13, 2020

@chrisjsewell , just want to make sure, is it going to help in excluding $ while copying the commands? Thanks

@chrisjsewell
Copy link
Member Author

is it going to help in excluding $ while copying the commands

Well give it a go, and see if it works as expected 😬 https://143-140029949-gh.circle-artifacts.com/0/html/index.html#documentation-builds

@championshuttler
Copy link

is it going to help in excluding $ while copying the commands

Well give it a go, and see if it works as expected 😬 https://143-140029949-gh.circle-artifacts.com/0/html/index.html#documentation-builds

We are already using it in firefox docs https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, thank you so much for this extension, its just new contributors copy paste as it is then they faced the problem, so it would be really helpful for a lot of people if somehow we can prevent $ to be copied

@chrisjsewell
Copy link
Member Author

We are already using it in firefox docs

Oh thats cool, but I think you misunderstand; the link I gave links to the documentation created using this PR, which (I believe) showcases exactly what you are after

@championshuttler
Copy link

We are already using it in firefox docs

Oh thats cool, but I think you misunderstand; the link I gave links to the documentation created using this PR, which (I believe) showcases exactly what you are after

oops my bad, just clicked on copy button, yeah it worked perfectly, thank you so much for doing it :)

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I really like this - infrastructure is much-improved, and the feature seems great to me. See one comment about using two lists for configuration rather than an "is_regex" flag - what do you think about that? Other than that, I think this is looking close to ready for merge

doc/conf.py Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
@@ -98,7 +98,8 @@
# html_sidebars = {}

# CopyButton configuration
copybutton_prompt_text = ">>> "
copybutton_prompt_text = ">>> |\\\\$ |\\[\\d*\\]: |\\.\\.\\.: "
Copy link
Member

Choose a reason for hiding this comment

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

One thought - could we simplify this a bit for people by allowing for a list instead of a single regexp? We could even allow for two configs like:

copybutton_prompt_start_strings = [">>>", "..."]
copybutton_prompt_start_regexes = ["\\[\\d*\\]"]

I suggest this because I think these should probably be the "defaults" anyway, and then if
people wanted to add a new "starting prompt" they can do so without needing to use regexes

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I can see some merit, but I guess the main thing I don't like is that it breaks back compatibility: I like that the current solution uses the original copybutton_prompt_text, then just adds a flag to turn on regexes.
Also it may not be as a compatible with what I think could be a next step, to improve robustness (as mentioned in #82 (comment)); to have a mapping of selectors to prompts, e.g something like.

copybutton_prompt_text = {
   "div higlight-console": "\\\\$ ",
   "div higlight-python": ">>> ",
   "div highlight-ipython": "\\[\\d*\\]: |\\.\\.\\.: "
}

Copy link
Member

Choose a reason for hiding this comment

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

hmm good point about backwards compatibility - we could keep the same original configuration and just add the regex one on top of it, and change behavior so that both will accept either a string (in which case it'll become a one-string list) or a list of strings?

for the dictionary extension, I agree that'd give us more precision. Do you think people will run into clashes between languages and starting characters?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could keep the same original configuration and just add the regex one on top of it, and change behavior so that both will accept either a string (in which case it'll become a one-string list) or a list of strings?

This feels more complicated than just using regexes lol? Especially given its only something they have to setup once, then never need to think about again.

Do you think people will run into clashes between languages and starting characters?

Well its probably not massively likely; really depends on people's own documentation. But I personally would rather take the time to add this in my/our repos, just to be sure

doc/conf.py Outdated Show resolved Hide resolved
@chrisjsewell
Copy link
Member Author

I really like this

thanks 😁

@choldgraf
Copy link
Member

choldgraf commented Jun 17, 2020

alright lets merge this in and we can decide if we want to add a "language: regexp" mapping later on.

I think one thing we can do is add a documentation section for "common regexes" since most people are not familiar with regex.

Thanks @chrisjsewell for the improvement, much better and thanks for going the extra mile and giving the testing infra the Chris S. treatment :-)

@choldgraf choldgraf merged commit 468be5c into master Jun 17, 2020
@choldgraf choldgraf deleted the prefix-regexes branch June 17, 2020 18:18
@chrisjsewell
Copy link
Member Author

alright lets merge this in and we can decide if we want to add a "language: regexp" mapping later on.

You read my mind 😄

and thanks for going the extra mile and giving the testing infra the Chris S. treatment

haha cheer, shame some certain others don't share my love of tests 👀 😆

@championshuttler
Copy link

championshuttler commented Jun 23, 2020

@chrisjsewell thanks for doing this i have a doubt do we have to add this https://github.com/executablebooks/sphinx-copybutton/blob/master/doc/conf.py#L101-L102 in our configuration file to prevent copying $

thanks

Edit - Got it thanks for it

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