-
Notifications
You must be signed in to change notification settings - Fork 49
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
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
1b5d3ea
Move format function to seperate file
chrisjsewell 58e277e
inject format func with jinja
chrisjsewell 9653cb7
Update integration.yml
chrisjsewell b1142bf
Update copybutton_funcs.js
chrisjsewell 4e7b5ee
Add javascript tests
chrisjsewell 91199d9
Add documentation on testing
chrisjsewell 3fc7c6e
fix code blocks
chrisjsewell b206f0a
Update integration.yml
chrisjsewell ffaffa5
parameterise js tests
chrisjsewell a7e53bc
simplify code
chrisjsewell 556e499
Add regexp matching
chrisjsewell d9cd423
Apply suggestions from code review
chrisjsewell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[flake8] | ||
max-line-length = 88 | ||
ignore=E203,W503 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,3 +108,5 @@ _build/ | |
|
||
# VS code config | ||
.vscode | ||
|
||
node_modules/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Install pre-commit hooks via | ||
# pre-commit install | ||
|
||
exclude: > | ||
(?x)^( | ||
\.vscode/settings\.json | ||
)$ | ||
|
||
repos: | ||
|
||
- repo: git://github.com/pre-commit/pre-commit-hooks | ||
rev: v2.2.3 | ||
hooks: | ||
- id: check-json | ||
- id: check-yaml | ||
- id: end-of-file-fixer | ||
- id: trailing-whitespace | ||
- id: flake8 | ||
|
||
- repo: https://github.com/psf/black | ||
rev: stable | ||
hooks: | ||
- id: black |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
sphinx | ||
ipython | ||
|
||
# Install ourselves | ||
. | ||
. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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