PR: Add troubleshooting blurb and OS version to issue template and enhance UI #6154
Conversation
|
Hello @CAM-Gerlach! Thanks for updating the PR.
Comment last updated on February 17, 2018 at 18:11 Hours UTC |
CAM-Gerlach
changed the title
|
And, yet another segfault on the See update above; I accidentally hit enter and it submitted it too soon, with no way for me to actually just delete it. |
The test I see failing is |
Yes, I just double checked it indeed has the latest |
ccordoba12
changed the title
spyder/app/mainwindow.py
Outdated
| "detail what you were doing when the error occurred. " | ||
| "Otherwise, your issue will likely be closed after 7 days. " | ||
| "Thanks.\n\n" | ||
| "**Note:** Delete this message before submitting." |
There was a problem hiding this comment.
Does this part go to the text published on Github when users press the Submit to Github button in our report errors dialog?
There was a problem hiding this comment.
Yes, so the apparent majority of users who are sending auto-generated error reports, or error reports from the Help menu, which appears to be the dominant source of "drive by" traceback only reports (rather than navigating to Github and hitting the "New Issue" button, where that text is filled from the .github issue template) will see it before they actually hit the submit their user-side/duplicate/incomplete issue and can get help faster. Or, if they don't remove it, we know they didn't even bother to read at it (as if they did, they would have seen the note saying it should be removed), giving stronger cause to summarily close it if desired.
Therein lies the problem, as mentioned above—due to the issue discussed in #5247 , at least in Firefox 52 ESR error reports are getting cut off around or below (due to % encoding) ~1300 real characters, and after editing it and the error template itself way down, perhaps more than you're comfortable with, that leaves around 100 characters of buffer after the dependencies are included. This should be okay for now for reports from the help menu, as they don't ever include more characters than this, and there's room for 2 or 3 more deps if need be, and to refine the format a bit more (maybe adding back a few words/headings here and there). Also, the change to the actual issue template is a non-issue since its done from Github, not Spyder.
However, this is a yuge problem for the autogenerated reports in many browsers, because it only leaves room for the shortest of tracebacks, and anything beyond that starts cutting off the dependencies, or (if long enough) the traceback itself. While this is already a problem (see #5247 ), this would exacerbate it.
Therefore, I suggest for now:
- Updating the Github issue template with the blurb, (currently implemented with this PR) as it is not affected
- Slimming down the existing basic structure of the Spyder-sent report where possible (currently implemented, and I can apply a few more tweaks to save a few more chars), to help a little more with traceback cutoff for now
- Not including the disclaimer (or maybe a one-line one for now) if a traceback is included, to leave room for the latter (will be implemented in this PR momentarily), otherwise, including it for now (implemented here)
- Including a interstitial dialog displaying the blurb if "Send to Github" is pressed from the error dialog, as originally proposed (future PR), or even better just integrating the text with the dialog from PR: Change error report dialog to enter issue description before submitting it #5346 (that other PR, or direct followup)
- Some better alternative to the current "send the full text of the template in a GET request through the browser" method, that either just sends the traceback and the dep versions, maybe encoded in a char saving format, and then adds the static parts later...somehow...or use the Github API, or send the former to a Snyder-team-controlled webpage that adds the latter, prompts the user for the description, and sends that to Github by a non-truncated means...but maybe that's complicating things. (In the future, and probably not by me, but hopefully soon-ish)
In any case, I will submit a new commit soon with the immediate changes mentioned above.
There was a problem hiding this comment.
Yes
Ok, so this needs PR #5346 to be merged first because @rlaverde discovered that many tracebacks are incomplete because it seems people are sending reports with Internet Explorer, which has a limit of 2000 chars in its URL bar.
Including a interstitial dialog displaying the blurb if "Send to Github"
Good idea. Please get the branch in PR #5346 and open a new PR with it and your new changes.
Some better alternative to the current "send the full text of the template in a GET request through the browser" method
Our idea is to force users to paste the contents of the error contained in the dialog into Github, instead of sending the whole error to the browser.
There was a problem hiding this comment.
Ok, so this needs PR #5346 to be merged first because @rlaverde discovered that many tracebacks are incomplete because it seems people are sending reports with Internet Explorer, which has a limit of 2000 chars in its URL bar.
Well, the original PR yes, eventually to accomplish all the above bullets, but not as it now stands. As discussed at (perhaps too much) length in my previous comment, I've revised what I have here to take that into account, so it is a flat improvement on the current situation for the time being:
- Adds the full error blurb to the semi-automated and manual Github reports with enough chars to spare for 3 additional dependencies even on the dev version (with a longer Spyder version code + SHA1),
- Has 46 more chars for tracebacks (vs. the current
3.x) with a one-liner version of the blurb - Includes an additional data point (OS Version), a
codeblock for the traceback, and refined formatting - Also helps mitigate the cut-off problem by a simple implementation of
Our idea is to force users to paste the contents of the error contained in the dialog into Github, instead of sending the whole error to the browser
And, crucially for getting merged promptly (ideally, for 3.2.6), it accomplishes this this without actually changing any existing UI text or program functionality, just the text/formatting of the error reports.
As mentioned, from my testing it appears this is also a problem on (at least) Firefox 52 ESR and Chrome on Windows 8.1 (which should natively support it); so it likely happens with all browsers on Windows. It seems many reports are cut off right now, especially the dependencies. Therefore, a 3.2.6 merge on this would help mitigate this until 3.2.7 or 3.2.8, if that's still possible.
There was a problem hiding this comment.
Please don't write so long comments on the revision thread but on the comments of the PR itself.
There was a problem hiding this comment.
Reasons for me to ask you do that:
- Your comments have very little to do with my simple question.
- Now I have to skip this long thread to do other revisions.
There was a problem hiding this comment.
I'm sorry; I was unaware of that convention. Would you like me to delete them and repost them on the main thread?
|
Also, is there a way we can just include it in this header?
|
Don't know, I guess you need to ask Github staff for that. |
|
@ccordoba12 Pushed a new version, as referenced in my latest reply in the review comment chain above, that implements what I discussed in my comment before your latest reply (and should also help address your concerns in that following comment). Here are some screenshots: Report Issue from Help MenuCurrent 3.x Send to Github Error Report3.x |
spyder/app/mainwindow.py
Outdated
| 12345678001234567800123456780012345678001234567800123456780012345678001234567800 | ||
| 12345678001234567800123456780012345678001234567800123456780012345678001234567800 | ||
| 12345678001234567800123456780012345678001234567800123456780012345678001234567800 | ||
| 12345678001234567800123456780012345678001234567800123456780012345678001234567800 |
There was a problem hiding this comment.
What is this big block doing here? Is it an image or something?
There was a problem hiding this comment.
More questions:
- Is this the SHA1 string you were talking about above?
- Could you better explain why this is needed?
- Could you move it to the beginning of this file as a constant? It could be called
SHA1_REPORT_ERRORor something like that. - Since this constant seems to be the same string repeated several times, please generate it using the multiplication properties of strings in Python, i.e.
SHA1_REPORT_ERROR = initial_string * 20.
There was a problem hiding this comment.
Its a total, rank, n00b mistake on my part; I must have somehow missed it despite usually being very careful to check git status and git diff every time before I commit. Its just a simple kludge for testing the maximum report length before cutoff that was never supposed to be committed, which happened despite several precautions I took to make sure I didn't screw up, as I often do. I'm sorry. Needless to say, it'll be gone in the next one.
spyder/app/mainwindow.py
Outdated
| issue_template = """\ | ||
| ## Description | ||
| %s |
There was a problem hiding this comment.
Let's use format here, instead of simple string replacement because now it's really hard to understand what each replacement corresponds to. So this should be (if I'm correct) {reminder}, instead of %s.
There was a problem hiding this comment.
Will do, thanks. I typically use format myself but didn't want to mess with the existing code without being instructed to or a strong reason.
spyder/app/mainwindow.py
Outdated
| **Please provide any additional information below** | ||
|
|
||
| ## Paste Traceback/Error Below | ||
| (from `View > Panels > Internal Console`) |
There was a problem hiding this comment.
I don't like this. The traceback should be copied automatically to the clipboard when users press Send to Github and they'd only need to press Ctrl+V or do a browser Paste here to paste it.
Besides, when the error dialog is active, tracebacks are not sent to the internal console.
There was a problem hiding this comment.
Okay, I will implement that as requested. A more general concern (if we don't include the tracebacks automatically) is that users, following previous behavior trends, will as often as not fail to paste them and thus we'll get nothing, but I suppose there is only one way to find out...
There was a problem hiding this comment.
Or will #5346 implement it instead? If so, what should to be done here (other than removing the ``(from View > ...) line)?
This PR still needs work, so I don't think it's ready for 3.2.6. As I said before, I don't want to rush this and it'd be better to merge PR #5346 first. @CAM-Gerlach, I really appreciate your work but we can make a new release whenever we want. I think 3.2.7 could only include these fixes and be released in three weeks at most, depending on how much work and time is put on it (I can move bugs not related to report errors improvements for 3.2.8). |
Absolutely, the right call for sure of course. I feel rather silly trying to push it, and I'm sorry I did so. Thanks for your understanding. |
|
Sorry for the delay; I was at the conference from 7AM through 10PM here without my laptop, though I did ask around about Spyder. In any case, implemented all the suggestions above, plus a test and some minor refactoring/cleanup. Let me know where you'd like me to go from here, to best integrate with #5346 . However, it now does not depend on anything in that PR to function or deliver an improved user experience from the get-go, but would certainly introduce merge conflicts with that PR, which would have to be fixed by rebasing one after the other is submitted. |
|
I think the best course of action is to finish #5346 first and then decide about any other improvements on top of that. The idea to finish that one is that after Submit to Github is pressed, users are sent to a new template page, where they should be asked to paste the contents of their clipboard. Those contents will be saved by us after pressing Submit to Github and they will include the description of the problem, all the necessary information (Operating system, release version, etc) and the full traceback. I am going to finish that PR myself because it's a bit tricky :-) |
|
Okay, sure thing—that makes a lot of sense.
Oh okay, so sort of like this idea?
I figured that could make sense for the longer term, but wasn't sure how realistic it was. Or do you mean something else? |
CAM-Gerlach
changed the title
CAM-Gerlach
force-pushed
the
github-template-troubleshooting
branch
from
228436e to
f29d326
Compare
CAM-Gerlach
changed the title
.github/ISSUE_TEMPLATE.md
Outdated
| @@ -1,28 +1,42 @@ | |||
| **PLEASE READ:** Before reporting here, *please* carefully read our **Troubleshooting Guide** at <https://github.com/spyder-ide/spyder/wiki/Troubleshooting-Guide-and-FAQ> and search the issues page here for your error/problem, as most postd bugs are duplicates or easy fixes. | |||
There was a problem hiding this comment.
I think we should enclose this paragraph in a markdown comment. I'd expect a lot of people forget to remove it.
There was a problem hiding this comment.
I would otherwise do so, but my reason behind making it visible and asking the user to delete it themselves is that it serves as a canary; if it show up not-deleted on a post, its an obvious sign to both us and the user that they very likely haven't bothered to read the "Please Read" section, which states quite clearly that they should delete it, and therefore further implies that they haven't done the basic due diligence of reading the troubleshooting guide or checking for other issues first. Furthermore, making it show up on the actual post gives the user more chances to see it, both in the preview and once they've posted it. However, if you still think I should comment it, then I will of course do so; let me know.
There was a problem hiding this comment.
It seems we're split in this decision. @spyder-ide/core-developers, @spyder-ide/junior-developers, what do you think about this?
There was a problem hiding this comment.
I agree with @ccordoba12, its most likely that people will forget to remove it from their troubleshooting reports
There was a problem hiding this comment.
I would like to see a screenshot first :P
There was a problem hiding this comment.
I vote for enclosing in markdown. Less work for everyone.
There was a problem hiding this comment.
I would also close incomplete reports right off the bat. Such reports never get additional info anyway.
There was a problem hiding this comment.
@CAM-Gerlach, I think the issue is settled now. Please enclose the necessary text in a markdown comment, so we can merge this one ASAP.
There was a problem hiding this comment.
@ccordoba12 Yes indeed, I've done so (and a few other fixes), and final CI tests are running now.
There was a problem hiding this comment.
@jnsebgosselin I certainly wouldn't say "never", but it does seem pretty rare for those that are just a traceback and not even a descriptive title. We could just close such "driveby" issues with our usual message, and ask the OP to reopen and comment if so. However, it is only a few minutes per day to close the status:Awaiting Followup issues as it is, and they're easy to keep track of, so it would only save a few minutes of my time per day at most, and we should see even fewer of them once this PR and its brethren go into effect, so its really a wash IMO.
.github/ISSUE_TEMPLATE.md
Outdated
| @@ -1,28 +1,42 @@ | |||
| **PLEASE READ:** Before reporting here, *please* carefully read our **Troubleshooting Guide** at <https://github.com/spyder-ide/spyder/wiki/Troubleshooting-Guide-and-FAQ> and search the issues page here for your error/problem, as most postd bugs are duplicates or easy fixes. | |||
|
|
|||
| If you do post an issue here, please describe step by step in detail what you were doing when the error occurred,. Otherwise, your issue will likely be closed after 7 days. Thanks. | |||
There was a problem hiding this comment.
This paragraph should be in a markdown comment too.
.github/ISSUE_TEMPLATE.md
Outdated
|
|
||
| If you do post an issue here, please describe step by step in detail what you were doing when the error occurred,. Otherwise, your issue will likely be closed after 7 days. Thanks. | ||
|
|
||
| **Note:** Delete this message before submitting. |
There was a problem hiding this comment.
Then we could remove this one, which is a bit misleading.
.github/ISSUE_TEMPLATE.md
Outdated
|
|
||
|
|
||
|
|
||
| ### Paste Traceback/Error Below (if applicable) |
There was a problem hiding this comment.
I don't think this section should be part of the issue template because that's why we have our new error dialog.
There was a problem hiding this comment.
This is just for the template, for users who report manually via Github, not our automatic system (e.g. if the error occurs on Spyder startup, they can't get it to launch at all, or they temporarily/permanently disabled the error dialog). Many of those cases would likely have a traceback, or at least an error message, and this would make it clear we need it if so. Let me know if you still want me to remove it.
There was a problem hiding this comment.
Ok, let's leave this section for now.
spyder/app/mainwindow.py
Outdated
| "If you don't find anything, enter a step-by-step description " | ||
| "(in English) of what led up to the problem; issue reports " | ||
| "without a clear way to reproduce them will be closed. Thanks.\n\n" | ||
| "**Note:** Please delete this message before submitting." |
There was a problem hiding this comment.
As I mentioned above, this should be in a markdown comment.
|
@CAM-Gerlach, everything looks pretty good! I just left some minor comments :-) |
|
@ccordoba12 Thanks for all the feedback; I'll take care of it as soon as I get home from work today. |
|
@ccordoba12 Hey, had some feedback for your feedback. Let me know what you'd like me to do, and if needed I'll push it ASAP. |
.github/ISSUE_TEMPLATE.md
Outdated
| @@ -1,28 +1,42 @@ | |||
| **PLEASE READ:** Before reporting here, *please* carefully read our **Troubleshooting Guide** at <https://github.com/spyder-ide/spyder/wiki/Troubleshooting-Guide-and-FAQ> and search the issues page here for your error/problem, as most postd bugs are duplicates or easy fixes. | |||
.github/ISSUE_TEMPLATE.md
Outdated
| @@ -1,28 +1,42 @@ | |||
| **PLEASE READ:** Before reporting here, *please* carefully read our **Troubleshooting Guide** at <https://github.com/spyder-ide/spyder/wiki/Troubleshooting-Guide-and-FAQ> and search the issues page here for your error/problem, as most postd bugs are duplicates or easy fixes. | |||
|
|
|||
| If you do post an issue here, please describe step by step in detail what you were doing when the error occurred,. Otherwise, your issue will likely be closed after 7 days. Thanks. | |||
There was a problem hiding this comment.
End of sentence - occurred,.
| @@ -2897,7 +2918,7 @@ def _check_updates_ready(self): | |||
| latest_release = self.worker_updates.latest_release | |||
| error_msg = self.worker_updates.error | |||
|
|
|||
| url_r = 'https://github.com/spyder-ide/spyder/releases' | |||
| url_r = __project_url__ + '/releases' | |||
There was a problem hiding this comment.
Sure CAM do, but any particular advantage in this context (one variable string, one fixed string) to offset the lesser code clarity stemming from loss of sequentiality and greater complexity?
| @@ -2459,7 +2477,7 @@ def report_issue(self, body=None, title=None): | |||
| if body is None: | |||
| body = self.render_issue() | |||
|
|
|||
| url = QUrl("https://github.com/spyder-ide/spyder/issues/new") | |||
| url = QUrl(__project_url__ + '/issues/new') | |||
There was a problem hiding this comment.
This may be a little nitpicky, but since string concatenation isn't encouraged, maybe use format strings?
I'm getting spoiled by 3.6, url = QUrl(f'{__project_url__}/issues/new'), even though Spyder can't use that yet.
There was a problem hiding this comment.
This may be a little nitpicky, but since string concatenation isn't encouraged, maybe use format strings?
Right, but isn't that just for concatenating more than two strings, and even then as a performance benefit on the order of microseconds unless you're concatenating a ton of strings or doing it in a loop? For two strings, both my own experience and what I've read has indicated a simple + is both clearest and just as fast.
I'm getting spoiled by 3.6,
url = QUrl(f'{__project_url__}/issues/new'), even though Spyder can't use that yet.
Same...
|
@ccordoba12 The Travis PyQt5 builds kept segfaulting at In any case, thanks all for your feedback. I Markdown-commented the message and removed the delete line, as agreed. I also made a few other similar instructions in the template into markdown comments, and uncommented a few lines that were inside backticks (and thus wouldn't work anyway), along with fixing a few inconsistencies/issues in the text and formatting between the various versions and the typos @csabella pointed out. Screenshots: Slightly revised error reporting dialog:
Github web error report (issue template):
Help menu "Report Issue" report:
Automatic issue report from new dialog:
|
|
@CAM-Gerlach, is this one ready? |
|
@ccordoba12 Yes. |
There was a problem hiding this comment.
Thanks for all your hard work on this one @CAM-Gerlach! Really great improvement!
|
Congrats! Great job. |
|
@csabella Thanks to you for spotting my silly typos, to @ccordoba12 for all his help, and @ccordoba12 and @rlaverde for doing most of the real work in their PRs. |
Essentially, just the Github issue template and auto-report format from #6137 to simplify that one, plus a few extra changes to harmonize the two somewhat divergenct templates, decrease wordiness, and minor cleanup, plus print OS version.
However, we have a significant problem: Likely similar to that described in #5247 , the length gets cut off after a certain point already with just the template and only a few lines of traceback, as despite the shortening elsewhere the overall length is still somewhat longer with the extra ~400 characters from the blurb at the top, allowing around 1300 chars total before truncating. (This was tested in Firefox 52 ESR), and in some rare cases depending on the actual length of the text, etc, the GET request fails completely, presumably because the split happened on some sort of critical char/boundary. And to be honest, this is nothing new; I see dep reports cut off all the time due to this, even if the user did everything else right.
However, I'm not sure how to fix this unless we switch to the Github API, or send the requests on to our own servers and then pass them on to Github...seems like we probably shouldn't merge this for the time being, though the actual EditTemplate.md is unaffected obviously.