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

Feedback Thread: Let me hear your thoughts #66

Closed
shalvah opened this issue Aug 10, 2020 · 34 comments
Closed

Feedback Thread: Let me hear your thoughts #66

shalvah opened this issue Aug 10, 2020 · 34 comments
Labels

Comments

@shalvah
Copy link
Contributor

shalvah commented Aug 10, 2020

Hi guys,

Just checking in with the community. If you've got a moment to spare, what's your experience with Scribe been like so far? What do you like? What don't you like? What do you think needs to be better?

@shalvah shalvah pinned this issue Aug 10, 2020
@shalvah
Copy link
Contributor Author

shalvah commented Aug 10, 2020

Btw, I recently launched Scribe for JS. It's going to support the most popular Node.js API frameworks, but at the moment it only supports Express, with Adonis.js in the works. It's nearly feature-compatible with Scribe, just less customisation features for now. Docs are here

@georgimorozov
Copy link
Contributor

georgimorozov commented Aug 26, 2020

Would there be an interest in the option to auto-generate the description for endpoints by using the bodyParams to generate markup? This is specifically for the postman collection file where associating descriptions with raw body data doesnt really work (all of our body data in postman is json data). I am in the process of implementing it on my branch, let me know if you think it would be useful and a PR is needed.

@shalvah
Copy link
Contributor Author

shalvah commented Aug 26, 2020

Can you provide an example @georgimorozov? Your explanation isn't very clear to me.

@rmundel
Copy link

rmundel commented Sep 17, 2020

Would be nice to have a cloud service to host multiple Scribe generated documentations, with teams/workspaces, analytics, etc.

@shalvah
Copy link
Contributor Author

shalvah commented Sep 17, 2020

There might be something in the works for that 🙂

@rmundel
Copy link

rmundel commented Sep 17, 2020

Please, hype us. 😆

@shalvah
Copy link
Contributor Author

shalvah commented Sep 25, 2020

A little update on v2: I'm done working on it. It comes with interactive functionality ("Try It Out"). You can see that here: https://scribe.readthedocs.io/en/v2/migrating-v2.html#changes-in-the-output. You can actually begin using it if you like, by setting your Composer constraint to dev-v2.

However, I just thought of a big change I'd like to make in v2, so I've created an RFC for that. #105

@konigbach
Copy link

Hi! First of all, thank you for your work! I have encounter a lot of improvements but, what I don't like it's that it generates bodyParams based on the formRequest rules method, when they are queryParams. Is there a way to avoid the autogeneration? Right now, I have to rollback because of this duplication. Thank you again!

@shalvah
Copy link
Contributor Author

shalvah commented Sep 28, 2020

Look in your config file and comment out the GetFromFormRequest strategy.

@konigbach
Copy link

Thank you!

@ajcastro
Copy link
Contributor

ajcastro commented Oct 14, 2020

I just saw this now after I already the issue.. Here is my feedback. Thanks!

#113

@kw-pr
Copy link

kw-pr commented Nov 30, 2020

In my code I had parameters named like the objects $article. That looks nice in my code but I did not like it in the URL: /article/{article}. I wanted to have /article/{article_id}.

To solve this I disabled Strategies\UrlParameters\GetFromLaravelAPI and wrote a new routeMatcher that always inserts _id in the parameter.

But I guess that is just me being silly. 😄

All in all I like Scripe a lot. Thanks for all the work!

@kw-pr
Copy link

kw-pr commented Dec 1, 2020

The print.css could get an update. I tried to print the page as pdf and the result was... Let's just say, I convinced everyone that we do not need the pdf. Webpage only is fine! 😉

@shalvah
Copy link
Contributor Author

shalvah commented Dec 8, 2020

The print.css could get an update

Ah, sorry, but I do not see that changing any time. It's something I copied from somewhere, and CSS is not something I fancy doing.

@shalvah
Copy link
Contributor Author

shalvah commented Dec 8, 2020

In my code I had parameters named like the objects $article. That looks nice in my code but I did not like it in the URL: /article/{article}. I wanted to have /article/{article_id}.

Yes, if you're using Laravel's resource routes, you'll get /article/{article} as your URL, so the parameter Scribe sees will be article. However, I think I'll make it smarter in v3, so it changes this to /article/{id}, since that's what API consumers would expect.

@shalvah
Copy link
Contributor Author

shalvah commented Dec 8, 2020

For the record, v3 is still in the planning phase, probably until January. Then hopefully, I can finish it by June/July.

@kw-pr
Copy link

kw-pr commented Dec 11, 2020

If you have more complex routes like /article/{id}/comment/{id} it will be difficult to document. That's why I just added _id in the route: /article/{article_id}/comment/{comment_id}.

@shalvah
Copy link
Contributor Author

shalvah commented Dec 11, 2020

If you have a route like /articles/{article}/comments/{comment}, Scribe won't generate /articles/{id}/comments/{id}. It will generate /articles/{article_id}/comments/{id}.

@kw-pr
Copy link

kw-pr commented Dec 11, 2020

I think that is inconsistent. But I see why you do it this way.

@tayeke
Copy link

tayeke commented Feb 9, 2021

I love love love this, thank you so much. I'm FINALLY getting comprehensive documentation of all our endpoints at work. I swear this package is almost perfect for me, however I can think of one area I need a lot more control. I really need to be able to control multiple examples. For example if I have two nested body params I need to define the corresponding example for each of the object[] generated. It always seems to be two objects generated, which is a great start for getting most of my docs built fast, but sometimes I need to have full and complete control over the examples. Something like in my example before if I could control each example objects values and omit optional params in one or the other.

/**
     * Update Store Hours
     *
     * @urlParam store_id integer required The Store ID. Example: 4
     * @bodyParam store_hours object[]
     * @bodyParam store_hours[].opening_time string required Time string for opening. Example_one: 06:00:00 Example_two: 08:30:00
     * @bodyParam store_hours[].closing_time string required Time string for closing. Example: Example_one 17:30:00 Example_two: 20:00:00
     * @bodyParam store_hours[].day_of_week string The day of the week you want to update the standard hours for. e.g. monday, tuesday, etc Example_one: tuesday
     * @bodyParam store_hours[].override_date string The date to override standard time for. If this is not null, the standard hours are not changed for this weekday e.g. 2020-11-10. Example_two: 2020-09-23
     */

@shalvah
Copy link
Contributor Author

shalvah commented Feb 11, 2021

Hmm. That's something of a limitation, as the Example: field only supports one example. Have you tried setting the example on the parent rather than the child items? In this case, something like:

/**
     * Update Store Hours
     *
     * @urlParam store_id integer required The Store ID. Example: 4
     * @bodyParam store_hours object[] Example: [{"opening_time": "06:00:00", "closing_time": "17:30:00", "day_of_week": "monday", "override_date": "2020-11-10"}, {"opening_time": "08:30:00", "closing_time": "20:00:00", "day_of_week": "tuesday", "override_date": "2020-09-23"}]
     * @bodyParam store_hours[].opening_time string required Time string for opening.
     * @bodyParam store_hours[].closing_time string required Time string for closing.
     * @bodyParam store_hours[].day_of_week string The day of the week you want to update the standard hours for. e.g. monday, tuesday, etc 
     * @bodyParam store_hours[].override_date string The date to override standard time for. If this is not null, the standard hours are not changed for this weekday e.g. 2020-11-10
     */

It's definitely quite cumbersome, but it might work.

@shalvah
Copy link
Contributor Author

shalvah commented Feb 11, 2021

It works for me (and you can even format the example nicely), but you have to remove the other fields though.

image

image

@shalvah
Copy link
Contributor Author

shalvah commented Feb 11, 2021

Can't promise this will change in v3, but I'll consider it.

@tayeke
Copy link

tayeke commented Feb 11, 2021

I had not thought of that. Thank you. I think the issue with having to remove the other items would be losing your documentation copy for each parameter in the object.

@danielbachhuber
Copy link

It'd be great to be able to specify the path for the generated Postman and OpenAPI docs. I've written my own routes for serving my API docs. Because Postman and OpenAPI are written to the local storage directory, they don't end up deployed to production.

@kw-pr
Copy link

kw-pr commented Mar 10, 2021

It'd be great to be able to specify the path for the generated Postman and OpenAPI docs. I've written my own routes for serving my API docs. Because Postman and OpenAPI are written to the local storage directory, they don't end up deployed to production.

I added php artisan scribe:generate to my deploy script.

This way production documentation is always up to dated.

@danielbachhuber
Copy link

@kw-pr I've installed scribe as a dev dependency to limit what's being deployed to production.

@chrisp-github
Copy link

chrisp-github commented Mar 17, 2021

I have been using this package for awhile now for API doc generation and really love it but the one area I am struggling with is Fractal transformers and there include system as I don't see a good way to document what the includes will look like.

I have managed to document what includes are available by having the following in the class doc block but would really love a way to also document what these includes would look like in an automatic way without having to define them myself.

 * <aside class="notice">
 *     Allowed includes
 *     <ul>
 *         <li>category</li>
 *         <li>skus</li>
 *         <li>design_props</li>
 *         <li>design_colours</li>
 *     </ul>
 * </aside>

@shalvah shalvah unpinned this issue May 24, 2021
@shalvah shalvah added the rfc label May 25, 2021
@Mina-R-Meshriky
Copy link

I think allowing for different levels of grouping would be a very nice feature to have
so now, its only one level of groups:

Users
    create a new user
    update a user

expected

V1
    Users
        create a new user
        update a new user
OR

V1 
    Admin Area Api's
        User Managment
            create a new user

@Mina-R-Meshriky
Copy link

also different auth can be used in the same project, so maybe for the admin area the authentication is done with laravel sanctum, some public Api's are authenticated by a bearer and others are with a key

@shalvah
Copy link
Contributor Author

shalvah commented Sep 28, 2021

The unfortunate thing is that Scribe can't meet every use case. Even if we could, it would add a whole bunch of complexity for most users, who don't need all that.

Features like multiple group levels and multiple authentication systems—It's unlikely we'd add that, because I see no way to implement them that won't add unnecessary complexity and possible confusion. If someone suggests a way to add such a feature while keeping things simple, then I might consider.

@shalvah
Copy link
Contributor Author

shalvah commented Sep 28, 2021

It'd be great to be able to specify the path for the generated Postman and OpenAPI docs. I've written my own routes for serving my API docs.

Yes, this is what you should do. We write to the local directory because it's simplest and most reliable, and you can easily add a script that moves it wherever you want and updates references.

@shalvah
Copy link
Contributor Author

shalvah commented Sep 28, 2021

Fractal transformers and there include system as I don't see a good way to document what the includes will look like.

There isn't any.


So here's the thing I want to emphasize: this is open-source. As I explained above, the package can't and won't support everything, sometimes because I can't think of a good approach. But if you can come up with a good proposal—not just an idea, but actually talk about how the implementation would work (and consider other people's use cases)—then I'd definitely consider it.

@shalvah
Copy link
Contributor Author

shalvah commented Sep 28, 2021

Closing this, but you can keep sharing feedback if you like.

@shalvah shalvah closed this as completed Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants