knuckleswtf / scribe

Generate API documentation for humans from your Laravel codebase.✍
https://scribe.knuckles.wtf/laravel/
MIT License
1.59k stars 280 forks source link

* When not using dot notation, overrides no longer remove all the ch… #728

Closed ProjectZero4 closed 9 months ago

ProjectZero4 commented 9 months ago

…ild keys in any parent array key. Instead, they are merged together.

Note: Dot notiation is still supported and I've also added support for the postman overrides too

Below i've put a screenshot of the overrides config and a before and after of the openapi.yaml file

OVERRIDES

image

BEFORE

image

AFTER

image
shalvah commented 9 months ago

That's fair, but I'm wondering what happens if you do want to replace it. After all, you can still achieve this by listing the keys manually, but if I accept this, you can no longer replace.

ProjectZero4 commented 9 months ago

The main thing that got me looking into this is that by opting into any overrides in this manner you immediately lose anything that was done by the package like the title and description.

Is there actually a use case for removing keys beyond setting them to null? i.e if you wanted to remove title you can do

'overrides' => [
    'info.title' => null,
],

OR

'overrides' => [
    'info' => [
        'title' => null,
        // other keys ...
    ],
],

I would expect the main use case for this particular feature would be to add as opposed to remove

What do you think?

shalvah commented 9 months ago

by opting into any overrides in this manner you immediately lose anything that was done by the package

Are you sure? The code uses data_set. Specifying info.version should set the version key on the info object only. You shouldn't lose any other info fields.

ProjectZero4 commented 9 months ago

Yeah to be explicit

On master if you had something like:

'overrides' => [
    'info' => [
        'version' => '2.4.1',
    ],
],

Then you will lose title and description on the generated yaml.

On master if you did the following:

'overrides' => [
    'info.version' => '2.4.1',
],

You would keep title and description

While I understand what you mean by:

you can still achieve this by listing the keys manually, but if I accept this, you can no longer replace.

The main question I think to answer is if the default behaviour should be to remove everything when using nested arrays or if it should only change what's specified in the config.

shalvah commented 9 months ago

That's all fine and good, but still, what's the point? Like, this works as expected:

'overrides' => [
    'info.version' => '2.4.1',
    'info.contact' => [
        'name' => '',
    ],
],

I believe in convenience, but I don't think being able to do

'overrides' => [
    'info' => [
        'version' => '2.4.1',
        'contact' => [
            'name' => '',
        ],
],

is such a step up.

Are there specific use cases I'm missing?

ProjectZero4 commented 9 months ago

Just done some research on Laravel's mergeConfigFrom and that behaves the same way as master. Happy to close since this change would break that consistency with laravel