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

Merge nested configurations rather than replacing them outright #676

Closed
jpsim opened this Issue May 27, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@jpsim
Collaborator

jpsim commented May 27, 2016

We currently just replace parent configurations with nested ones, but it'd be nicer and less confusing to users if nested configurations inherited their parents.

Current source: https://github.com/realm/SwiftLint/blob/0.10.0/Source/SwiftLintFramework/Models/Configuration.swift#L220-L225

Currently merge simply overrides the current configuration with the new configuration.
This requires that all configuration files be fully specified. In the future this should be
improved to do a more intelligent merge allowing for partial nested configurations.

See #675 for one case of such confusion.

@dgommers

This comment has been minimized.

Show comment
Hide comment
@dgommers

dgommers Dec 6, 2016

We were also facing this behaviour within our project. I would like to help by implementing this mini-feature. But before going there, I am considering an specific approach and I would like to see if this matches with other SwiftLint maintainers' vision.

Instead of implementing the placeholder merge method on the Configuration struct, I would like to propose to perform it on a lower level. This will make the implementation more scalable and responsive to any changes on content level.

More specific:

  1. Parse configuration files directly into a dictionary, instead of Configuration struct, which is currently implemented inside configurationForPath method (recursively).
  2. Merge-in each dictionary, not requiring any knowledge about the actual contents (improves scalability).
  3. Initialise the Configuration struct based on the combined dictionary.

Thanks!

dgommers commented Dec 6, 2016

We were also facing this behaviour within our project. I would like to help by implementing this mini-feature. But before going there, I am considering an specific approach and I would like to see if this matches with other SwiftLint maintainers' vision.

Instead of implementing the placeholder merge method on the Configuration struct, I would like to propose to perform it on a lower level. This will make the implementation more scalable and responsive to any changes on content level.

More specific:

  1. Parse configuration files directly into a dictionary, instead of Configuration struct, which is currently implemented inside configurationForPath method (recursively).
  2. Merge-in each dictionary, not requiring any knowledge about the actual contents (improves scalability).
  3. Initialise the Configuration struct based on the combined dictionary.

Thanks!

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Dec 7, 2016

Collaborator

I don't think that's the right way to do this. For one, merging two Configuration structs has the advantage of dealing with strongly-typed values, and to avoid parsing/interpreting the untyped YAML dictionary. For another, we already have Configuration values that we need to construct for the subsets of the files on which they apply.

Collaborator

jpsim commented Dec 7, 2016

I don't think that's the right way to do this. For one, merging two Configuration structs has the advantage of dealing with strongly-typed values, and to avoid parsing/interpreting the untyped YAML dictionary. For another, we already have Configuration values that we need to construct for the subsets of the files on which they apply.

@ManWithBear

This comment has been minimized.

Show comment
Hide comment
@ManWithBear

ManWithBear Apr 15, 2017

Hi, I have started to implement this feature.
Do you have any ideas how it should work?
Right now I think to separate Configuration rules in 4 categories Disabled, Configured, Custom, Default. And combine it like: child configured rules fully override parent configured/default rules with same ID. And after that combine rules together and remove all disabled by parent or child.

ManWithBear commented Apr 15, 2017

Hi, I have started to implement this feature.
Do you have any ideas how it should work?
Right now I think to separate Configuration rules in 4 categories Disabled, Configured, Custom, Default. And combine it like: child configured rules fully override parent configured/default rules with same ID. And after that combine rules together and remove all disabled by parent or child.

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim May 24, 2017

Collaborator

Hi @ManWithBear , I think it'd be best to file a PR with the behavior your think is best, and we can review/adapt from there.

Collaborator

jpsim commented May 24, 2017

Hi @ManWithBear , I think it'd be best to file a PR with the behavior your think is best, and we can review/adapt from there.

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Aug 2, 2017

Collaborator

Thanks @stephanecopin and @jpsim 💯

Collaborator

marcelofabri commented Aug 2, 2017

Thanks @stephanecopin and @jpsim 💯

@andrewtheis

This comment has been minimized.

Show comment
Hide comment
@andrewtheis

andrewtheis Jan 24, 2018

This feature seems to have broken cases where you wish the nested config for a rule to override the settings in the parent.

andrewtheis commented Jan 24, 2018

This feature seems to have broken cases where you wish the nested config for a rule to override the settings in the parent.

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Apr 8, 2018

Collaborator

@andrewtheis can you be a bit more specific? Please file an issue with more details about what you want compared to what the current behavior is.

Collaborator

jpsim commented Apr 8, 2018

@andrewtheis can you be a bit more specific? Please file an issue with more details about what you want compared to what the current behavior is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment