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

style-guide: put token guidelines into sections & add varargs #6200

Merged
merged 5 commits into from
Sep 12, 2021
Merged

style-guide: put token guidelines into sections & add varargs #6200

merged 5 commits into from
Sep 12, 2021

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Jul 6, 2021

Fixes: #5791

@xeruf
Copy link
Contributor Author

xeruf commented Jul 6, 2021

It was quite unwieldy, now it feels much more manageable to me :)

@xeruf xeruf changed the title style-guide: add varargs & prune wording style-guide: put token guidelines into sections & add varargs Jul 6, 2021
@marchersimon marchersimon added the documentation Issues/PRs modifying the documentation. label Jul 6, 2021
marchersimon
marchersimon reviewed Jul 6, 2021
View reviewed changes
Copy link
Collaborator

@marchersimon marchersimon left a comment

Choose a reason for hiding this comment

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

Just a few small suggestions, but looks good overall

contributing-guides/style-guide.md Outdated Show resolved Hide resolved
contributing-guides/style-guide.md Outdated Show resolved Hide resolved
contributing-guides/style-guide.md Outdated Show resolved Hide resolved
@marchersimon
Copy link
Collaborator

@xeruf are you still working on this?

@xeruf
Copy link
Contributor Author

xeruf commented Jul 23, 2021

jup, but I haven't been able to keep up with github notifications recently ^^

marchersimon
marchersimon reviewed Jul 24, 2021
View reviewed changes
Copy link
Collaborator

@marchersimon marchersimon left a comment

Choose a reason for hiding this comment

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

Just two more things I'd like to add

contributing-guides/style-guide.md Show resolved Hide resolved
contributing-guides/style-guide.md Outdated Show resolved Hide resolved
@blueskyson blueskyson mentioned this pull request Aug 1, 2021
Merged
@github-actions
Copy link

Hi all! This thread has not had any recent activity.
Are there any updates? Thanks!

@github-actions github-actions bot added the waiting Issues/PRs with Pending response by the author. label Aug 12, 2021
@marchersimon marchersimon removed the waiting Issues/PRs with Pending response by the author. label Aug 13, 2021
@github-actions
Copy link

Hi all! This thread has not had any recent activity.
Are there any updates? Thanks!

@github-actions github-actions bot added the waiting Issues/PRs with Pending response by the author. label Aug 29, 2021
@marchersimon marchersimon removed the waiting Issues/PRs with Pending response by the author. label Sep 1, 2021
@blueskyson
Copy link
Member

LGTM. but I personally wonder that Is there any reason to replace list marks from number to -?

@marchersimon
Copy link
Collaborator

I actually think this would be a benefit, since now we can refer to a specific rule by it's number.

@xeruf
Copy link
Contributor Author

xeruf commented Sep 6, 2021

They had no particular order, and it would be annoying to adjust the numbers when a rule is inserted - also the referencing would misalign after a change.

Two ideas to solve this:

  • Numbering per section
  • Add a shorthand that identifies each rule, e.g. SHO, CAS, VAL for the three naming rules
    Latter is my favorite as it is more future-proof.

@marchersimon
Copy link
Collaborator

I don't think this would be necessary. We don't have to refer to the style-guide that often and we can still refer to the line number or the section

marchersimon
marchersimon approved these changes Sep 7, 2021
View reviewed changes
Copy link
Collaborator

@marchersimon marchersimon 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, thanks @xeruf

CleanMachine1
CleanMachine1 approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@CleanMachine1 CleanMachine1 left a comment

Choose a reason for hiding this comment

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

lgtm, this PR is way overdue

@marchersimon marchersimon merged commit ec4a9a4 into tldr-pages:main Sep 12, 2021
@xeruf xeruf deleted the patch-1 branch September 18, 2021 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CleanMachine1 CleanMachine1 CleanMachine1 approved these changes

@marchersimon marchersimon marchersimon approved these changes

@sbrl sbrl Awaiting requested review from sbrl sbrl is a code owner

@blueskyson blueskyson Awaiting requested review from blueskyson

Assignees
No one assigned
Labels
documentation Issues/PRs modifying the documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Style question: What about variable number of arguments (varargs)?
4 participants
@xeruf @marchersimon @blueskyson @CleanMachine1