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

[workflow] Update issue templates #1794

Merged
merged 7 commits into from
Sep 7, 2020

Conversation

archibate
Copy link
Collaborator

Related issue = #677

[Click here for the format server]


Preview the affect of config.yml: https://github.com/pypa/pypi-support/issues/new/choose
Should I add refactor.md in this PR?

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #1794 into master will decrease coverage by 0.89%.
The diff coverage is 10.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1794      +/-   ##
==========================================
- Coverage   43.35%   42.45%   -0.90%     
==========================================
  Files          43       44       +1     
  Lines        5995     6202     +207     
  Branches     1040     1073      +33     
==========================================
+ Hits         2599     2633      +34     
- Misses       3246     3414     +168     
- Partials      150      155       +5     
Impacted Files Coverage Δ
python/taichi/core/__init__.py 0.00% <ø> (ø)
python/taichi/core/util.py 0.37% <0.00%> (+0.08%) ⬆️
python/taichi/lang/__init__.py 42.44% <0.00%> (-11.16%) ⬇️
python/taichi/lang/shell.py 0.00% <0.00%> (ø)
python/taichi/lang/util.py 31.36% <0.00%> (ø)
python/taichi/misc/gui.py 0.00% <0.00%> (ø)
python/taichi/testing.py 75.75% <0.00%> (-2.37%) ⬇️
python/taichi/tools/np2ply.py 0.00% <0.00%> (ø)
python/taichi/main.py 23.03% <4.76%> (-0.45%) ⬇️
python/taichi/code_format.py 12.37% <10.71%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de26d08...cc47915. Read the comment docs.

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

A few nits

.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.gitignore Outdated
@@ -74,4 +74,3 @@ _build
*.ptx
*.ll
*.bc
*.yml
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for removing this? Asking because action recording could produce yml files (which IMO should be ignored).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.travis.yml, appveyor.yml, etc. There're too many files involving yml. For example, I added .github/workflows/some_new_workflow.yml, and find it's not added because of .gitignore.
FYI I use .gitignore_localrc for ignoring these outputs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see

Copy link
Member

Choose a reason for hiding this comment

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

I believe we should keep ignoring *.yml files. Most of these should not be added automatically.
You can always use git add --force for exceptions.
Please revert this change if that makes sense.

.github/ISSUE_TEMPLATE/feature_request.md Outdated Show resolved Hide resolved
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM

@archibate archibate added the LGTM label Aug 28, 2020
Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Cool! Just some comments. Thanks.

.gitignore Outdated
@@ -74,4 +74,3 @@ _build
*.ptx
*.ll
*.bc
*.yml
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should keep ignoring *.yml files. Most of these should not be added automatically.
You can always use git add --force for exceptions.
Please revert this change if that makes sense.

.github/ISSUE_TEMPLATE/config.yml Outdated Show resolved Hide resolved

**Describe the solution you'd like (if any)**
A clear and concise description of what you want to achieve and implement.
You may also describe an alternative solution you've thought (if any).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You may also describe an alternative solution you've thought (if any).

Sorry but I don't understand this - do you want the issue opener to describe a second solution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, better expressions?

Comment on lines 11 to 22
A clear and concise description of what you want. For example:

I would like to add an Apple Metal backend to the compiler so that my Macbook GPU can be utilized.

**Think about the advantages and disadvantages of the proposed feature**
All coins have its two side, we encourage people to think critically. For example:

Advantages:
1. So that Apple users could utilize their GPU resources.

Disadvantages:
1. Increases the maintainance cost over multiple backends.
Copy link
Member

Choose a reason for hiding this comment

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

The older version actually looks better. It's good to think about "two sides", but we don't want to overwhelm newcomers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about Think about the advantages and disadvantages of the proposed feature (if any)? For example I immediately closed #1648 after thought about not-return-by-reference. This is actually helpful for people to rethink the reason why we need to add this feature and search for better solutions.

@yuanming-hu
Copy link
Member

yuanming-hu commented Aug 28, 2020

Should I add refactor.md in this PR?

Smaller PRs are always good. We should do that in a future PR. The format of #1750 may serve as a reference. Thanks! :-)

@archibate archibate removed the LGTM label Sep 6, 2020
Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Cool! LGTM. Thank you very much!

@archibate archibate added the LGTM label Sep 7, 2020
@archibate archibate merged commit 4308985 into taichi-dev:master Sep 7, 2020
@yuanming-hu yuanming-hu mentioned this pull request Sep 9, 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