Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Conversation

atwixfirster
Copy link
Contributor

@atwixfirster atwixfirster commented Jan 31, 2020

@devops-devdocs
Copy link
Collaborator

An admin must run tests on this PR before it can be merged.

1 similar comment
@devops-devdocs
Copy link
Collaborator

An admin must run tests on this PR before it can be merged.

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @atwixfirster. Thank you for your collaboration. The example is very useful. In order to answer all questions from the original issue, we need to add a little bit more information. Please, check the following suggestions.

It would be great if we leave a reference in this topic to the following topic so developers will know where to go for the detailed information.

Then, the following topic describes the general idea of the configuration file merge.
It would be awesome if we could have a few sentences about how to see (debug) the merged configuration. So, the question is if a developer wants to verify that a custom configuration is present in the merged config, what he is going to do?

Thank you!

@shrielenee
Copy link
Contributor

Thanks for picking this up @atwixfirster!

@atwixfirster
Copy link
Contributor Author

atwixfirster commented Feb 3, 2020

Thanks for picking this up @atwixfirster!

Hi, @shrielenee !

Looks like my PR has pushed @diazwatson to work on the #3361 again. :)

So, to avoid a double work I will probably stop my work on this one and will wait for the @diazwatson updates (see PR #6516).

Thank you!

@shrielenee
Copy link
Contributor

Whoever wants to do it is fine by us! @atwixfirster @diazwatson It was just such an easy grab... so you can fight it out over this one 👊 😄

@diazwatson
Copy link
Contributor

@atwixfirster https://www.youtube.com/watch?v=2NUn0HxvEDU

@atwixfirster
Copy link
Contributor Author

It would be great if we leave a reference in this topic to the following topic so developers will know where to go for the detailed information.

The appropriate link has been added.

Then, the following topic describes the general idea of the configuration file merge. It would be awesome if we could have a few sentences about how to see (debug) the merged configuration. So, the question is if a developer wants to verify that a custom configuration is present in the merged config, what he is going to do?

The appropriated callout block has been added with reference to debug entry point.

@rogyar , could you please review?

Thank you!

1 similar comment
@atwixfirster
Copy link
Contributor Author

It would be great if we leave a reference in this topic to the following topic so developers will know where to go for the detailed information.

The appropriate link has been added.

Then, the following topic describes the general idea of the configuration file merge. It would be awesome if we could have a few sentences about how to see (debug) the merged configuration. So, the question is if a developer wants to verify that a custom configuration is present in the merged config, what he is going to do?

The appropriated callout block has been added with reference to debug entry point.

@rogyar , could you please review?

Thank you!

@ghost
Copy link

ghost commented Feb 7, 2020

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@atwixfirster atwixfirster reopened this Feb 7, 2020
@dobooth dobooth requested a review from rogyar February 10, 2020 19:50
@dobooth dobooth added 2.3.x Magento 2.3 related changes Major Update Significant original updates to existing content labels Feb 10, 2020
@rogyar
Copy link
Contributor

rogyar commented Feb 11, 2020

From a technical perspective, the information in the current PR is correct. However, we have another PR with more extended information on the same matter.

#6539

The advantage of the current PR is the creation time. The advantage of the PR mentioned above is richer information.

I'm not sure about the fair decision here. We could either ask @atwixfirster to extend info in this PR and accept it as a preceding one or accept the last one from @diazwatson.

@dobooth could you assist in this tricky decision, please? :)

Thank you!

@dobooth
Copy link
Contributor

dobooth commented Feb 11, 2020

Happy to arbitrate here, gents. This is the kind of debate we like! Thanks to @rogyar for having a great awareness of what's happening in the repo. And thanks to you all for your continued efforts. I am going to go with #6539 since it does seem to have a bit more data. I will head over there and clean up those linting errors... 😉

@rogyar
Copy link
Contributor

rogyar commented Feb 12, 2020

Hi @dobooth. Thanks a lot for your arbitration 👍

@dobooth
Copy link
Contributor

dobooth commented Feb 18, 2020

Closing. This was handled in another PR.

@dobooth dobooth closed this Feb 18, 2020
@ghost
Copy link

ghost commented Feb 18, 2020

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2.3.x Magento 2.3 related changes Major Update Significant original updates to existing content Partner: Atwix partners-contribution PR created by Magento partner Progress: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration object topic
7 participants