apisyouwonthate / style-guide

A shared and somewhat opinionated style guide for everyone to enjoy.
141 stars 20 forks source link

Document the reasoning behind rules #40

Closed Tristan971 closed 1 year ago

Tristan971 commented 1 year ago

Hello,

As I was looking to lint our OpenAPI description with your style guide, I was a bit surprised, specifically by this rule:

 51:7   warning  api-home                   Stop forcing all API consumers to visit documentation for basic interactions when the API could do that itself.           paths

It's not entirely clear what it means besides perhaps encourage some godawful Json blob serving as minimal documentation (and also locking up / for it rather than for a swagger/redoc/whatever minisite). While I don't see massive value in that versus a well-documented API, I'm rather surprised that going that route isn't encouraged using HAL or similar things (given the authors of the rule seem to believe this is any desirable, at least).

And in general, a bunch of other rules are rather cryptic in intent, notably:

 51:7   warning  api-health                 Creating a `/health` endpoint is a simple solution for pull-based monitoring and manually checking the status of an API.  paths

when we already expose a GET /ping route for this for example. Is /health better than /ping? Perhaps? What about /healthz that most modern projects use to avoid using a fairly common path that a service might need to expose actual domain data?

And in general, it's a bit weird to see such authoritative tone to the error messages when the project doesn't document its reasoning anywhere reasonably easy to find (neither on your website, nor this repo's readme, nor in the ruleset source...).

philsturgeon commented 1 year ago

Hey @Tristan971, thanks for the feedback!

These rules are mostly letting you know about common standards for REST, the sort that can be found on our subsite https://standards.rest.

If you've been reading the book and blogs then none of these things will be a big surprise to you, but if you're coming at this fresh then of course it'll be a little confusing. The ruleset can do more to explain itself, and mostly the descriptions do that for now:

https://github.com/apisyouwonthate/style-guide/blob/main/src/ruleset.ts#L63

I'll put a little more time and effort into the descriptions now that I know people are out there trying to use it.

Mostly I've been working on these, all of which have descriptions for why, and hosted documentation somewhere so you can see those explanations.

Not sure where that documentation will go for this one, but I'm sure I'll find some free time between all the million other things im doing, running a reforestation charity, writing three books, etc. 😉

Tristan971 commented 1 year ago

If you've been reading the book and blogs then none of these things will be a big surprise to you, but if you're coming at this fresh then of course it'll be a little confusing.

Yup that was the main thing; if there were links to the relevant site/blog articles it'd be much easier to get the reasoning. I was not aware of these 2 IETF specs before (though even after reading I'd question their practicality, but that's another topic altogether).

Mostly I've been working on these, all of which have descriptions for why, and hosted documentation somewhere so you can see those explanations.

Yeah so we use these others too already. Or at least I ran them and kept some (the owasp one for example makes a few assertions that are somewhat debatable imo, but that's off-topic).

philsturgeon commented 1 year ago

Looking forward to the pull requests to OWASP friend! 🙌🏻

Tristan971 commented 1 year ago

idk if I worded it badly or not, but if came off as that negative I didn't mean for it to do so 🤷 but I'm happy to raise a few issues with the pieces I find objection to, to see people's opinion on the matter

philsturgeon commented 1 year ago

Had a chance to look into this today thanks to Stoplight.io giving me time to work on it.

It seems the message and description on some of these was switched around, so the wrong thing was showing up in the wrong place. I've made the messages very clear about what the problem is, and the descriptions now explain more about why and provide links to more information.

So far thats only showing up the ruleset.ts, so I need to figure out how to document this. I'll make a new isseu for that.

Thank you!