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

[Github] Upgrading ISSUE_TEMPLATE to display 4 alternatives: Bug, Regression, Question or Feature Request #21

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Conversation

Sajjon
Copy link
Contributor

@Sajjon Sajjon commented Feb 5, 2020

Upgrading ISSUE_TEMPLATE from single markdown to 4 option selection: Bug, Regression, Question, Feature Request

Motivation:

Having this template where when user presses NEW ISSUE button she has to think about what she's doing might lead to better issues submitted.

Modifications:

  1. Replaced ISSUE_TEMPLATE.md with directory containing four different issues types.
  2. Added new script ./scripts/environment.sh which puts to together a summary of users environment and copies to pasteboard (asking first).

Result:

New-Issue alternatives

new_issue_options

Bug report

With having pasted output generated from ./scripts/environment.sh
bug_report

Feature request

Kind of like the same as before
feature_request

@swift-server-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

In general I like this idea. I think we need to extend the pasteboard script to behave sensibly on other platforms as well. Can we add the output of uname -a as well? That will nicely cover getting information about other OSes.

@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

@Lukasa I've updated the script a bit, to check if we are on macOS, if so detect which version and Xcode info (might be useful, right?). The script looks like this now:

Script

read -p "This will replace your current pasteboard. Continue? [y/n]" -n 1 -r
echo    # (optional) move to a new line
if [[ $REPLY =~ ^[Yy]$ ]]
then
	swiftversion=$(swift --version)
	unix_version_name=$(uname -a)
	i="${i}Swift version: ${swiftversion}\n"
	i="${i}Unix version: ${unix_version_name}\n"

	# Check if OS is macOS, if so retrieve which version and Xcode version.
	if [[ "$OSTYPE" == "darwin"* ]]; then
		macos=$(defaults read loginwindow SystemVersionStampAsString | cat -)
		xcodebuild_version=$(/usr/bin/xcodebuild -version | grep Xcode)
		xcodebuild_build=$(/usr/bin/xcodebuild -version | grep Build)
		xcodeselectpath=$(xcode-select -p | cat -)
	
		i="${i}macOS version: ${macos}\n"
		i="${i}Xcode-select path: '${xcodeselectpath}\n"
		i="${i}Xcode: ${xcodebuild_version} (${xcodebuild_build})"
	fi

	echo "${i}" | pbcopy
	echo "Your pasteboard now contains debug info, paste it on Github"
fi

Output

Hmm for some reason my newlines inside the if does not work... Do you have an idea of what is causing that? (the second line is very long..., contains Xcode info, but newline does not work.)

Swift version: Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin19.3.0\nUnix version: Darwin Sajjode.local 19.3.0 Darwin Kernel Version 19.3.0: Thu Jan  9 20:58:23 PST 2020; root:xnu-6153.81.5~1/RELEASE_X86_64 x86_64\nmacOS version: 10.15.3\nXcode-select path: '/Applications/Xcode.app/Contents/Developer\nXcode: Xcode 11.3.1 (Build version 11C505)

Issue template

Do you want me to change anything in the markdown templates? Or are they good?

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I think the templates are all good, and the new script seems fine to me: I have one note about the copyright header and otherwise we're good to go.

##
## This source file is part of the SwiftCrypto open source project
##
## Copyright (c) 2019 Apple Inc. and the SwiftCrypto project authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment should be updated to be 2020.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, also fixed in script I added yesterday scripts/generate_boilerplate_files_with_gyb.sh

I also updated reference to CONTRIBUTORS.txt, which is now CONTRIBUTORS.md

@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

@Lukasa I fixed the comment in license (year + reference to CONTRIBUTORS.txt). I also fixed newline issue in ./script/environment.sh, by adding the magic -e to echo. This is what the script produces:

Swift version: Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin19.3.0
Unix version: Darwin Sajjode.local 19.3.0 Darwin Kernel Version 19.3.0: Thu Jan  9 20:58:23 PST 2020
 root:xnu-6153.81.5~1/RELEASE_X86_64 x86_64

macOS version: 10.15.3
Xcode-select path: '/Applications/Xcode.app/Contents/Developer
Xcode: Xcode 11.3.1 (Build version 11C505)

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks!

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

@swift-server-bot test this please

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Looks like some C files got missed. In practice you probably have to change almost every header comment due to the rename of the contributors file.

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

It may be worth doing the header comment fixup in a separate PR just to keep the scope clear.

…ssues types. Adding script for extraction of enviroment info, in order to make it easier for users to report issues.
@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

@Lukasa True, good idea, I will split them into two separate PRs. I've updated this to only include Github issue template + new script environments.sh.

So ready to be merged I think?

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Yup, LGTM. Let's see if the robot agrees.

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

@swift-server-bot test this please

@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

For future reference: Regarding discussions of changes of file headers, see #24

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

Ah, we need to update the sanity script. Find the line that does sed -e 's/2018-2019/YEARS/' -e 's/2019/YEARS/' and change it to sed -e 's/2018-2019/YEARS/' -e 's/2019/YEARS/' -e 's/2020/YEARS/'

@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

@Lukasa like so? be6e8e9

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

Looks right

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

@swift-server-bot test this please

scripts/environment.sh Outdated Show resolved Hide resolved
@Sajjon
Copy link
Contributor Author

Sajjon commented Feb 6, 2020

I might be too liberal about my use of git push -f.... but did not wanna spam git history with trivial fixes. I tend to like git commit -amend. So, it ought to be... amended now :D

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

@swift-server-bot test this please

@Lukasa Lukasa merged commit 3be1b8c into apple:master Feb 6, 2020
@Lukasa
Copy link
Collaborator

Lukasa commented Feb 6, 2020

🎉

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