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

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

Conversation

ProjectZero4
Copy link
Contributor

@ProjectZero4 ProjectZero4 commented Sep 14, 2023

…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

…ild keys in any parent array key. Instead, they are merged together.
@shalvah
Copy link
Contributor

shalvah commented Sep 14, 2023

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
Copy link
Contributor Author

ProjectZero4 commented Sep 14, 2023

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
Copy link
Contributor

shalvah commented Sep 14, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

shalvah commented Sep 14, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants