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

Fix formatting inconsistencies #192

Merged
merged 7 commits into from
May 16, 2017
Merged

Fix formatting inconsistencies #192

merged 7 commits into from
May 16, 2017

Conversation

mattmcneeney
Copy link
Contributor

The spec had become a bit inconsistent, so this is a sweeping change to tidy it up a bit and make it pretty. Let me know what you think and LGTM if happy, thanks!

@cfdreddbot
Copy link

Hey mattmcneeney!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@angarg12
Copy link
Contributor

Should we wait for the other spec changes that are about to be accepted to rebase and merge this one?

@mattmcneeney
Copy link
Contributor Author

Yeah good idea, it would probably best to merge imminent ones and then rebase this one before getting LGTMs. I'll bump this in a day or so.

@duglin
Copy link
Contributor

duglin commented May 10, 2017

SGTM and yeah let's wait for the other PRs to go first since it'll be rebase hell and some of these fixes are part of those PRs too.

@mattmcneeney mattmcneeney mentioned this pull request May 12, 2017
Merged
@mattmcneeney
Copy link
Contributor Author

Had another run through this in attempt to avoid rebase hell. Let's get any imminent PRs merged so we can sort this one out! 🙏

@mattmcneeney mattmcneeney mentioned this pull request May 12, 2017
Merged
duglin
duglin reviewed May 12, 2017
View reviewed changes
spec.md Outdated
@@ -40,7 +40,7 @@ A platform marketplace MAY expose services from one or many service brokers, and

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
"SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space before "MAY". should just search for double-spaces in the entire doc. :-)

duglin
duglin reviewed May 12, 2017
View reviewed changes
spec.md
| id* | string | An identifier used to correlate this service in future requests to the broker. This MUST be globally unique within a platform marketplace. Using a GUID is RECOMMENDED. |
| description* | string | A short description of the service. |
| tags | array-of-strings | Tags provide a flexible mechanism to expose a classification, attribute, or base technology of a service, enabling equivalent services to be swapped out without changes to dependent logic in applications, buildpacks, or other services. E.g. mysql, relational, redis, key-value, caching, messaging, amqp. |
| requires | array-of-strings | A list of permissions that the user would have to give the service, if they provision it. The only permissions currently supported are <code>syslog\_drain</code>, <code>route\_forwarding</code> and <code>volume\_mount</code>. |
Copy link
Contributor

Choose a reason for hiding this comment

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

very happy to see these "code" blocks being replace with back-ticks!

@duglin
Copy link
Contributor

duglin commented May 12, 2017 •
edited by chipchilders
Loading

one minor double-space missed but I want this merged ASAP, so
LGTM

Approved with PullApprove

@duglin
Copy link
Contributor

duglin commented May 12, 2017

@mattmcneeney let's do the 80 column wrap in a follow-on PR

@angarg12
Copy link
Contributor

angarg12 commented May 12, 2017 •
edited by chipchilders
Loading

LGTM!

Approved with PullApprove

@shalako
Copy link
Contributor

shalako commented May 12, 2017 •
edited
Loading

My expectation is this will break CF docs, as:

  • pound signs on both sides of the title are used for auto-generating table of contents
  • backticks are not recognized in tables, so html is necessary

This may or may not still be the case.
@mattmcneeney, please check with the docs team if you haven't already. Thanks!

@vaikas
Copy link
Contributor

vaikas commented May 13, 2017 •
edited by chipchilders
Loading

LGTM

Approved with PullApprove

@mattmcneeney
Copy link
Contributor Author

@duglin thanks for the heads up. I'll check with the CF docs team asap, so let's not merge this one just yet. In the meantime, I'm about to push a fix for the double spaces @duglin

@duglin
Copy link
Contributor

duglin commented May 13, 2017

I have a feeling that we may run into a lot of problems in the future if we try to continue down this path of our spec being in markdown as well as an html-md-hybrid that the CF doc tooling needs. Someone once mentioned the name of the tool that CF uses but I can't remember -what is it? I'd like to explore the option of helping the CF guys out by creating a tool that will take a "normal" github md docs and twiddle it so that their tooling will work - similar to what I did in #168 (comment) . Then we could stick to just github md.

@avade
Copy link
Contributor

avade commented May 14, 2017

@duglin https://github.com/pivotal-cf/bookbinder is the tool. @bentarnoff would be the person to speak to, but I don't expect it would be a small piece of work.

@avade
Copy link
Contributor

avade commented May 14, 2017

Is this good for another round of LG's?

@duglin
Copy link
Contributor

duglin commented May 14, 2017

@avade what about @shalako's comment? Are CF docs ok with this?

@duglin
Copy link
Contributor

duglin commented May 14, 2017

@bentarnoff @avade I noticed this https://github.com/pivotal-cf/bookbinder#middleman-templating-helpers I wonder if we could create a helper to tweak the MD into something bookbinder would be ok with? Or, if not, what does it use to pull down the repo? If its just "git" in the path then we could wrap git with a bash script to twiddle things after the clone.

@avade
Copy link
Contributor

avade commented May 15, 2017

Let's wait to hear from @bentarnoff before merging.

CF docs are the only place the spec is rendered at the moment.

@mattmcneeney
Copy link
Contributor Author

I've checked out the CF docs locally and have been playing around with what we can and can't change as of today. Results as follows:

  • Backticks in tables are fine
  • Navigation is broken with or without #'s on the other side of titles and doesn't affect ToC (they have been broken since the anchor tags were removed a while ago, and we don't use bookbinder to generate ToC for the API docs)
  • Using two stars for bold text (**Text**) is not supported inside <p class="note">
  • Three backticks appears differently to <pre class="terminal">

@bentarnoff LMK what you think of this list. If we could fix navigation and, if possible, change a pair of three backticks to render <pre class="terminal">...content...</pre> either in CI or through a custom middleman helper then we can get this spec tidied up.

@mattmcneeney
Copy link
Contributor Author

I've submitted a PR to the bookbinder repo that fixes the navigation. We can make do with the <pre class="terminal"></pre> for now and hopefully get this merged once the docs PR gets approved.

@mattmcneeney
Copy link
Contributor Author

We've removed all HTML tags now except <p class="note"> as we don't have a way in markdown yet to do nice things like that. The rest of the spec now is pretty pure though.

@mattmcneeney
Copy link
Contributor Author

@bentarnoff We're happy with the way the docs look as long as we can merge this bookbinder PR that fixes markdown heading anchors. Let me know if you're happy to merge that (or point me towards who I have to speak to) and then we can be done here!

@bentarnoff
Copy link

Let me bring in @animatedmax, who can speak to the bookbinder PR.

Per @avade's request, CF docs is now pulling from the v2.11 branch of this repo. But it doesn't appear to have solved the ToC issue, which you can see is still a problem on our OSS staging site: https://docs-cloudfoundry-staging.cfapps.io/services/api.html

@animatedmax
Copy link

The bookbinder PR appears to resolve the anchor issue. It adds an extra id to the anchors we manually add to pages, but I believe they have no appreciable impact on these existing pages.

@mattmcneeney
Copy link
Contributor Author

mattmcneeney commented May 15, 2017 via email

@angarg12
Copy link
Contributor

angarg12 commented May 15, 2017 •
edited by chipchilders
Loading

LGTM!

Approved with PullApprove

@mattmcneeney
Copy link
Contributor Author

Three more please 🥇 @avade @duglin @pmorie and co

@shalako
Copy link
Contributor

shalako commented May 16, 2017 via email

@avade
Copy link
Contributor

avade commented May 16, 2017 •
edited by chipchilders
Loading

LGTM

Approved with PullApprove

1 similar comment
@vaikas
Copy link
Contributor

vaikas commented May 16, 2017 •
edited by chipchilders
Loading

LGTM

Approved with PullApprove

@vaikas vaikas merged commit 1370d45 into openservicebrokerapi:master May 16, 2017
@mattmcneeney mattmcneeney deleted the tidy-up branch May 16, 2017 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duglin duglin duglin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mattmcneeney @cfdreddbot @angarg12 @duglin @shalako @vaikas @avade @bentarnoff @animatedmax