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

PR: Add checklists for Github issue report and pull requests #6870

Merged
merged 5 commits into from
May 7, 2018

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Mar 31, 2018

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 3.x)
  • Followed PEP8 for code style
  • Ensured your pull request hasn't eliminated unrelated blank lines/spaces,
    modified the spyder/defaults directory, or added new icons/assets
  • Wrote at least one-line docstrings for any new functions
  • Added at least one unit test covering the changes, if at all possible
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed
  • Included a screenshot, if this PR makes any visible changes to the UI

Description of Changes

This PR adds a checklist for the user to complete before submitting an issue report (both in the issue template and submitted from the Spyder Help menu; automatic/semi-auto error reports are unaffected), to hopefully help increase the quality of issues received and enable speedy closure of those that aren't helpful. To avoid any chance of an incomplete report due to length limitations, the text verbosity for the Help menu report (particularly the mostly redundant reminder) was cut down where possible.

image

Also, it adds a heavily revised PR template (as used here) with a checklist (top), along with formatting enhancements, some additional items and helpful tips in the comments.

Issue(s) Resolved

As this is an internal change to the Github templates, none believed needed.

@CAM-Gerlach CAM-Gerlach added this to the v3.2.9 milestone Mar 31, 2018
@CAM-Gerlach CAM-Gerlach self-assigned this Mar 31, 2018
@CAM-Gerlach CAM-Gerlach requested a review from ccordoba12 March 31, 2018 05:03
@CAM-Gerlach
Copy link
Member Author

@ccordoba12 Need anything from me to get this merged?

@CAM-Gerlach
Copy link
Member Author

@ccordoba12 ?

@ccordoba12
Copy link
Member

I think there are way too many boxes to check. Could you reduce the list a bit?

@CAM-Gerlach CAM-Gerlach force-pushed the add-github-checklists branch from 1ad1985 to a7bd365 Compare April 17, 2018 03:50
@CAM-Gerlach
Copy link
Member Author

I think there are way too many boxes to check. Could you reduce the list a bit?

Okay, sure. I reduced the number of boxes in the issue report template by 1/3, and considerably cut down most of the text. The Help menu semi automated report version is even shorter still. Also, I cut the pull request one down by 1/4 and similarly eliminated some extra text. Let me know what you think,

@CAM-Gerlach CAM-Gerlach force-pushed the add-github-checklists branch from a7bd365 to 9f9342e Compare April 17, 2018 03:57
@CAM-Gerlach CAM-Gerlach force-pushed the add-github-checklists branch from 9f9342e to abac344 Compare April 18, 2018 18:28
@CAM-Gerlach
Copy link
Member Author

@ccordoba12 Any further changes you'd like?

* Python version:
* Qt version:
* PyQt version:
* OS name and version:
Copy link
Member

Choose a reason for hiding this comment

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

OS is a term some people could not understand. So please revert this to Operating System:

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Apr 23, 2018

Choose a reason for hiding this comment

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

The problem (and I think I mentioned this before as well) is that we're out of characters to play around with, as aside from a small buffer I built in for one or two deps (and IIRC Linux/Mac has one more line for psutil than the Windows I tested it on, and there's an issue to add Matplotlib as a dep) , the report starts getting cut off or outright producing errors when trying to send it to github, and I already cut and cut and cut the issue checklist down to under 1/3 of its original length as well as the disclaimer, headers etc . The real issue is that the dependency list takes up ~3-4x the characters it should due to all the fancy symbols used needed percent encoding; even a small change here (eliminating one or two deps that rarely matter; using one or two fewer unnecessary symbols in the plain text version, etc. will cut dozens or even hundreds of characters from the final URL.

Please advise what you'd like me to do—fix the plain text display of the deps? Convert this to use the new error dialog? Just risk it and add back the chars? If I do the latter, adding even one more dep (as is discussed elsewhere) will likely break the whole URL based error reporting mechanism, or at least come dangerously close.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know what you'd like me to do about this, @ccordoba12 . Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay in coming back to you. Please remove this entry in the new list:

Completed the problem description, steps to reproduce, ...

That's kind of obvious and it should be enough to add the characters that we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the confusion; I meant to make the above initial comment on the mainwindow review item instead. That entry only exists (in that lengthy form at least) in the issue report template; the relevant text for the Help menu Report Issue item is in mainwindow. However,

Completed all applicable sections below

while much shorter, does exist and, once percent encoded, is indeed around the same length as a dependency line, much longer than "versions". Unfortunately it isn't as obvious as one would think as many reporters don't do it, but at least it is something we can easily see for ourselves. I'll go ahead and eliminate it and spell out versions and OS, and push the changes momentarily; that should at least give us a little breathing room for the time being.

* Qt version: {qt_version}
* {qt_api_name} version: {qt_api_version}
* Operating system: {os_name} {os_version}
* Spyder ver: {spyder_version} {commit}
Copy link
Member

Choose a reason for hiding this comment

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

I think we have talked about this before. ver can be confusing and is not explicit enough for non-English speakers. So please revert this to version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, explicit is better than implicit, and I don't often use abbreviations myself. See above for the reasons and alternatives.

* Python ver: {python_version}
* Qt ver: {qt_version}
* {qt_api_name} ver: {qt_api_version}
* OS: {os_name} {os_version}
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this to Operating System.

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Apr 23, 2018

Choose a reason for hiding this comment

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

See above.

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented May 6, 2018

@ccordoba12 Please let me know how you'd like to handle the length situation; thanks.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @CAM-Gerlach!

@ccordoba12 ccordoba12 merged commit 3a5c118 into spyder-ide:3.x May 7, 2018
ccordoba12 added a commit that referenced this pull request May 7, 2018
@CAM-Gerlach CAM-Gerlach deleted the add-github-checklists branch May 7, 2018 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants