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

Update docs to match new markdown style guide. #6863

Closed
wants to merge 1 commit into from

Conversation

4 participants
@cirosantilli
Copy link
Contributor

commented Apr 24, 2014

Now that we have a markdown style guide I've started updating the docs to match that guide.

I have updated part of the docs. Please check that this is OK, and if yes I will finish updating the rest of the guide.

If you don't agree with the style in guide, please open an issue / pull request directly at: https://github.com/cirosantilli/markdown-styleguide for us to discuss and link to it from here.

The main changes include:

@cirosantilli cirosantilli changed the title [WIP please review] Update docs to match new markdown style guide. [WIP][waiting for review] Update docs to match new markdown style guide. Apr 27, 2014

PROCESS.md Outdated
- Green labels `#009800`: issues that can generally be ignored. For example, issues given the following labels normally can be closed immediately:
- Feature request (see copy & paste response: [Feature requests](#feature-requests))
- Support (see copy & paste response: [Support requests and configuration questions](#support-requests-and-configuration-questions)
- Light orange `#fef2c0`: workflow labels for issue team members (awaiting feedback, awaiting confirmation of fix)

This comment has been minimized.

Copy link
@jvanbaarsen

jvanbaarsen Apr 28, 2014

Contributor

What are you putting 3 spaces here? since its not a paragraph, it should be one space right?

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli Apr 28, 2014

Author Contributor

My idea was:

  1. "green labels" contains an inner list, so it should get 3 spaces (same if it contained an inner code block).

  2. It looks nicer and might be more potable if all elements of the list have the same spacing, that is:

-   light orange
-   green labels
    - inner list

Instead of:

- light orange
-   green labels
    - inner list

Both 1) and 2) are hard to decide on points (lists are super underspecified) and if the community prefers another style we can discuss it.

This comment has been minimized.

Copy link
@jvanbaarsen

jvanbaarsen Apr 28, 2014

Contributor

But why not just:

- light orange
- green labels
   - inner list

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli Apr 28, 2014

Author Contributor

For uniformity between code blocks and other inner elements. For codeblocks this is important for portability as explained at: http://www.cirosantilli.com/markdown-styleguide/#rationale-why-not-always-single-space, so I though it would make more sense and be safer to use the same rule for all inner elements like lists.

@@ -10,7 +10,7 @@ GET /projects/:id/repository/branches

Parameters:

+ `id` (required) - The ID of a project

This comment has been minimized.

Copy link
@jvanbaarsen

jvanbaarsen Apr 28, 2014

Contributor

Note to self: Continue review here

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2014

Thanks for your contribution, one request, can you please push a little smaller MR's next time? That way its easier to review. This is going to take a little time to review.

@dblessing Can you help :-)

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2014

I understand that reviewing this is a bit messy, but I think it makes sense to make this a single mass refactor commit.

Someday I will write a script based on a hacked Redcarpet parser that auto checks / auto corrects where possible.

+ `ref` (required) - Create branch from commit sha or existing branch
- `id` (required) - The ID of a project
- `branch_name` (required) - The name of the branch
- `ref` (required) - Create branch from commit sha or existing branch

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Extra spaces on the 2 lines above. They were there before, too, but might as well clean them up here.

+ `assignee_id` (optional) - Assignee user ID
+ `title` (required) - Title of MR
+ `target_project_id` (optional) - The target project (numeric id)
- `id` (required) - The ID of a project

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Fix indentation of the description. Also pre-existing but a minor format fix that would be good for this commit.

6. Select "Register application".
7. You should now see a Client ID and Client Secret near the top right of the page (see screenshot). Keep this page open as you continue configuration. ![GitHub app](github_app.png)
8. On your GitLab server, open the configuration file.
1. Sign in to GitHub.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Incremental numbering in this doc seems to be broken here. I think because of whitespace? Sometimes the numbering starts over.

* Authorized redirect URI: 'https://gitlab.example.com/users/auth/google_oauth2/callback'
9. Under the heading "Client ID for web application" you should see a Client ID and Client secret (see screenshot). Keep this page open as you continue configuration. ![Google app](google_app.png)
10. On your GitLab server, open the configuration file.
1. Sign in to the [Google Developers Console](https://console.developers.google.com/) with the Google account you want to use to register GitLab.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Numbering also wonky here.


### Supported Providers
1. Choose one or more of the Supported Providers below to continue configuration.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Numbering also an issue here.

3. Select "Create new app"
4. Fill in the application details.
* Name: This can be anything. Consider something like "\<Organization\>'s GitLab" or "\<Your Name\>'s GitLab" or
To enable the Twitter OmniAuth provider you must register your application with Twitter. Twitter will generate a client ID and secret key for you to use.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Numbering in this file, too :)

You accept and agree to the following terms and conditions for Your present and future Contributions submitted to GitLab.com. Except for the license granted herein to GitLab.com and recipients of software distributed by GitLab.com, You reserve all right, title, and interest in and to Your Contributions.

1. Definitions.
1. Definitions.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Numbering in this file is written as 1, 2, 3 and so on as opposed to the 1, 1, 1, 1 scheme that is preferred. Is there a reason for that or just a typo?

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli Apr 29, 2014

Author Contributor

I'm in doubt about this particular case: since its a legal thing, people are likely to refer to points by their number from external documents. Rationale at: http://www.cirosantilli.com/markdown-styleguide/#ordered . Once again, hard choice that could be reviewed.

This comment has been minimized.

Copy link
@dosire

dosire May 1, 2014

Member

I like the 1,2,3 since it is a legal thing.

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli May 1, 2014

Author Contributor

Sure, I had even modified the style in view of that file =)

You accept and agree to the following terms and conditions for Your present and future Contributions submitted to GitLab.com. Except for the license granted herein to GitLab.com and recipients of software distributed by GitLab.com, You reserve all right, title, and interest in and to Your Contributions.

1. Definitions.
1. Definitions.

This comment has been minimized.

Copy link
@dblessing

dblessing Apr 29, 2014

Member

Same numbering question as above - 1, 2, 3 and opposed to 1, 1, 1

This comment has been minimized.

Copy link
@dosire

dosire May 1, 2014

Member

Same remark, 1,2,3 is OK since it is legal.

@dblessing

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

I made an initial pass on everything - just a few formatting issues. The numbering seems to be a challenge and I'm concerned about the different parsers we have - Github, GitLab, and the new doc site - doc.gitlab.com. I hope all are in agreement on these changes, especially with numbering.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

Thanks for the review @dblessing, all comments were useful. I'll have a look at the numbering issues.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

I consider the numbering issue a GitHub bug as explained in detail at: isaacs/github#181

Lets see how GitHub replies, and maybe start thinking how the styleguide can work around this ... One possibility is not using empty lines as it was before.

@dblessing

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

@cirosantilli Some of the changes you made with numbering have extra lines in between and some don't. What's the differentiator there?

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

@dblessing

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

Ok. Since it works at the moment to not have the empty lines, can you revert those parts which have problems? We can revisit if/when GitHub fixes.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2014

Yes, that's probably the best way. I'll do it on next push.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2014

Can I assume that the new style is OK and continue with the conversion?

@dblessing

This comment has been minimized.

Copy link
Member

commented May 1, 2014

@dosire How does this look so far? We've given some suggestions and @cirosantilli wonders if we're comfortable enough that work can continue.

@dosire

This comment has been minimized.

Copy link
Member

commented May 1, 2014

@dblessing @cirosantilli I love the work so far, I could not spot any mistakes, very well done, please continue.

@dosire

This comment has been minimized.

Copy link
Member

commented May 1, 2014

@dblessing Thanks for helping to review this.

@cirosantilli cirosantilli changed the title [WIP][waiting for review] Update docs to match new markdown style guide. [WIP] Update docs to match new markdown style guide. May 1, 2014

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2014

  1. Corrected every typo I saw. Typos are bad style =).

  2. Please check the README very carefully.

    I understand that the README is a critical file and that you may wish to use special rules, so if you disagree just tell me and I'll revert away.

    • split "GitLab: Self hosted Git management software" into two headers logo because with a header one it wouldn't fit on a single line on GitHub.
    • transformed "GitLab is open source software to collaborate on code" to "Open source software to collaborate on code" or it wouldn't fit on a single line on GitHub.
    • some changes make punctuation more consistent with the other markdown files:
      • colon before every code block:

        Code block coming:

        code
        
      • separte definition like lists from content with some kind of punctuation. E.g.:

        GitLab Chef Cookbook This cookbook can be used
        

        To:

        GitLab Chef Cookbook. This cookbook can be used
        
  3. Under raketasks:

    • modified README to match all given top level headers
    • renamed backup restore as Backup Restore would make a bad top level header, and the file does not talk only about restoring, but also creating backups
+ [Maintenance](maintenance.md)
+ [User management](user_management.md)
+ [Web hooks](web_hooks.md)
- [backup](backup_restore.md)

This comment has been minimized.

Copy link
@dosire

dosire May 2, 2014

Member

Shouldn't this be [backup](backup.md) ?

This comment has been minimized.

Copy link
@dosire

dosire May 2, 2014

Member

Can't we name it Backup and restore?

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli May 2, 2014

Author Contributor

Done.

+ [User management](user_management.md)
+ [Web hooks](web_hooks.md)
- [backup](backup_restore.md)
- [cleanup](cleanup.md)

This comment has been minimized.

Copy link
@dosire

dosire May 2, 2014

Member

Why is the capital gone here and for backup?

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli May 2, 2014

Author Contributor

The idea was using the exact name of the task, but maybe upper case is better, updating.

@dosire

This comment has been minimized.

Copy link
Member

commented May 2, 2014

README.md looks nice, thanks

+ [Maintenance](maintenance.md)
+ [User management](user_management.md)
+ [Web hooks](web_hooks.md)
- [Backup and restore](backup.md)

This comment has been minimized.

Copy link
@dosire

dosire May 2, 2014

Member

Please undo the rename, backup_restore.md is an OK file name.

@dblessing

This comment has been minimized.

Copy link
Member

commented May 2, 2014

@dosire @jvanbaarsen did a good bit of the initial review also. Team effort :)

@dosire

This comment has been minimized.

Copy link
Member

commented May 2, 2014

@cirosantilli cirosantilli changed the title [WIP] Update docs to match new markdown style guide. Update docs to match new markdown style guide. May 4, 2014

@dblessing

This comment has been minimized.

Copy link
Member

commented May 7, 2014

@cirosantilli Do you have any other changes you'd like to submit? If not, can you please make the adjustment dosire mentioned and change the filename back to backup_restore.md?

And rebase :)

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

I'm done. I had undone the filename but forgot to undo the link =)

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2014

@cirosantilli Ok thanks! Can you rebase?

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

Oops, forgot. Doing it.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

Done. Also added top level headers to all READMEs, as some had and others not.

@@ -84,23 +85,20 @@ After making the release branch new commits are cherry-picked from master. When
- Push VERSION + Tag to master, merge into x-x-stable
- Publish blog for new release
- Tweet to blog (see below)
* 22th: release GitLab EE

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli May 7, 2014

Author Contributor

Why are there two 22th?

This comment has been minimized.

Copy link
@dosire

dosire May 14, 2014

Member

Thanks, this didn't make any sense and is now removed in https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/release/monthly.md

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

Found spelling errors at doc/release/monthly.md: 20st and 23nd. Also see line comment above.

@dblessing

This comment has been minimized.

Copy link
Member

commented May 7, 2014

👍 good catch @cirosantilli.

Not sure about the multiple 22nd's. I'd say reduce to one, or maybe @dosire can comment if the date is different for EE.

@dosire

This comment has been minimized.

Copy link
Member

commented May 14, 2014

I think https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/release/monthly.md is without the mentioned spelling errors now.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented May 30, 2014

@cirosantilli What is the status of this PR?

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2014

When you are ready to merge soon tell me and I'll update. It has to be done quickly as the refactor is huge and it is very likely that new commits introduce conflicts. When I was last prompted to make mergeable I did it =). Shall I do it again now?

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented May 31, 2014

Well, im not the one who manages the merges. @randx can you let him know when you're able to merge this?

@dosire

This comment has been minimized.

Copy link
Member

commented Jun 2, 2014

Hi @cirosantilli and @jvanbaarsen, @randx authorized me to merge this, if you make it mergable I'll try to merge it within 24 hours.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2014

@dosire Awesome, thanks!

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2014

I deleted my fork during some housekeeping and don't know how to reattach the branch to this PR.

Made a new mergeable PR at: #7067 doing the exact same thing as this one, and closing this one now.

If anyone knows what could be done instead in this case, please tell me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.