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.

Sign up for GitHub

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

Jump to bottom

README // Add formatting policy. #3680

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

README // Add formatting policy. #3680

wants to merge 9 commits into from

Conversation

ShikiSuen
Copy link
Contributor

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@ShikiSuen ShikiSuen requested a review from madsmtm as a code owner May 4, 2024 11:36
@ShikiSuen ShikiSuen mentioned this pull request May 4, 2024
Merged
5 tasks
@ShikiSuen
Copy link
Contributor Author

Force-pushed in order to remove contents of PR#3619.

@pan93412
Copy link

pan93412 commented May 4, 2024 •
edited

It may be better to add this section in CONTRIBUTING.md.

@ShikiSuen
Copy link
Contributor Author

It may be better to add this section in CONTRIBUTING.md.

Then it needs more discussion since this CONTRIBUTING.md defines guidelines from a macro perspective. This PR addesses things from a micro perspective in detail.

@kchibisov
Copy link
Member

This should go to CONTRIBUTING.md into ## CONTRIBUTING with a new ### section. README shouldn't educate how to submit patches.

Besides, the maintainers guide in contributing do mention that maintainer (me) must ensure formatting and help out, which I offer in case you don't want to do something.

@kchibisov kchibisov requested review from daxpedda and notgull May 4, 2024 15:38
@ShikiSuen
Copy link
Contributor Author

This should go to CONTRIBUTING.md into ## CONTRIBUTING with a new ### section. README shouldn't educate how to submit patches.

Besides, the maintainers guide in contributing do mention that maintainer (me) must ensure formatting and help out, which I offer in case you don't want to do something.

I moved them to the CONTRIBUTING.md but left a shortcut in README.md.

kchibisov
kchibisov reviewed May 4, 2024
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Changes should go after ### Submitting your work in CONTRIBUTING.md.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
kchibisov
kchibisov reviewed May 4, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@ShikiSuen ShikiSuen requested a review from kchibisov May 4, 2024 16:00
kchibisov
kchibisov reviewed May 4, 2024
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ShikiSuen @kchibisov
Add extra notes from Chibisov.
0c81d65 
Co-Authored-By: Kirill Chibisov <contact@kchibisov.com>
@ShikiSuen ShikiSuen requested a review from kchibisov May 4, 2024 16:05
@ShikiSuen @kchibisov
CONTRIBUTING // Remove redundant Event.
33ad523 
Co-Authored-By: Kirill Chibisov <contact@kchibisov.com>
kchibisov
kchibisov requested changes May 4, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ShikiSuen @kchibisov
Address line-limit issues.
3019264 
Co-Authored-By: Kirill Chibisov <contact@kchibisov.com>
@ShikiSuen ShikiSuen requested a review from kchibisov May 4, 2024 16:23
kchibisov
kchibisov approved these changes May 4, 2024
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

fine by me, could add 80 chars limit note I've said, but I won't block on it or can send myself.

@ShikiSuen
Copy link
Contributor Author

fine by me, could add 80 chars limit note I've said, but I won't block on it or can send myself.

I am afraid that the fastest approach is to let you handle it. This saves time from reviewing.

@kchibisov
Copy link
Member

I am afraid that the fastest approach is to let you handle it. This saves time from reviewing.

Sure, done.

I'll handle the rest of the review then as well.

madsmtm
madsmtm approved these changes May 5, 2024
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Am fine with this both with and without my nits, thanks!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ShikiSuen @madsmtm
Apply suggestions from code review by madsmtm.
49b0794 
Co-authored-by: Mads Marquart <mads@marquart.dk>
@ShikiSuen
Copy link
Contributor Author

ShikiSuen commented May 5, 2024 •
edited

Report: CI failed in check documentation. I have no clue what is happening.

Amend Report: All nightly CI tests failed, requiring assistance.

@madsmtm
Copy link
Member

madsmtm commented May 5, 2024

I've opened #3682 for that

daxpedda
daxpedda approved these changes May 20, 2024
View reviewed changes
CONTRIBUTING.md
Comment on lines +54 to +56
Winit relies on nightly `rustfmt`. A PR will usually not get reviewed if the
nightly `cargo-fmt` check did not pass. If you have `rustup` installed you can
install the nightly toolchain using `rustup toolchain install nightly`.
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
Winit relies on nightly `rustfmt`. A PR will usually not get reviewed if the
nightly `cargo-fmt` check did not pass. If you have `rustup` installed you can
install the nightly toolchain using `rustup toolchain install nightly`.
Winit relies on nightly `rustfmt`. If you have `rustup` installed you can
install the nightly toolchain using `rustup toolchain install nightly`.

I don't believe this is necessary.

CONTRIBUTING.md
Comment on lines +62 to +64
If you feel uncomfortable using a `nightly` toolchain, ensure that CI passes
with the exception of formatting, and ask the maintainers on your PR to format
the code before merge.
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to encode this into a kind of rule, I think we are capable enough to make judgements like this individually.

I'm also not sure why anybody would be "uncomfortable" using a nightly toolchain, considering its just for formatting. Installing the nightly toolchain does not force you to use or ship compiled binaries with nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid that survivalship bias does not always work, sir.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@ShikiSuen @daxpedda
Update CONTRIBUTING.md
a3a6fe0 
Co-authored-by: daxpedda <daxpedda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daxpedda daxpedda daxpedda approved these changes

@notgull notgull Awaiting requested review from notgull

@madsmtm madsmtm Awaiting requested review from madsmtm

@kchibisov kchibisov Awaiting requested review from kchibisov

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ShikiSuen @pan93412 @kchibisov @madsmtm @daxpedda