Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix using incorrect config file #1604

Merged
merged 2 commits into from
Jun 13, 2017
Merged

Fix using incorrect config file #1604

merged 2 commits into from
Jun 13, 2017

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Jun 3, 2017

Fixes #1531.

@jpsim jpsim requested a review from marcelofabri June 3, 2017 00:53
@SwiftLintBot
Copy link

SwiftLintBot commented Jun 3, 2017

12 Messages
📖 Linting Aerial with this PR took 0.36s vs 0.37s on master (2% faster)
📖 Linting Alamofire with this PR took 2.33s vs 2.49s on master (6% faster)
📖 Linting Firefox with this PR took 10.28s vs 11.36s on master (9% faster)
📖 Linting Kickstarter with this PR took 13.72s vs 14.43s on master (4% faster)
📖 Linting Moya with this PR took 0.73s vs 0.75s on master (2% faster)
📖 Linting Nimble with this PR took 1.38s vs 1.41s on master (2% faster)
📖 Linting Quick with this PR took 0.44s vs 0.46s on master (4% faster)
📖 Linting Realm with this PR took 2.12s vs 2.15s on master (1% faster)
📖 Linting SourceKitten with this PR took 0.94s vs 0.89s on master (5% slower)
📖 Linting Sourcery with this PR took 2.88s vs 2.94s on master (2% faster)
📖 Linting Swift with this PR took 9.38s vs 9.71s on master (3% faster)
📖 Linting WordPress with this PR took 10.43s vs 10.05s on master (3% slower)

Generated by 🚫 Danger

@codecov-io
Copy link

codecov-io commented Jun 3, 2017

Codecov Report

Merging #1604 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1604      +/-   ##
==========================================
- Coverage    88.2%   88.18%   -0.02%     
==========================================
  Files         194      194              
  Lines        9680     9693      +13     
==========================================
+ Hits         8538     8548      +10     
- Misses       1142     1145       +3
Impacted Files Coverage Δ
...urce/SwiftLintFramework/Models/Configuration.swift 85.23% <100%> (-1.1%) ⬇️
...s/SwiftLintFrameworkTests/ConfigurationTests.swift 94.24% <100%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8eae6e...dbdc9f7. Read the comment docs.

Copy link
Collaborator

@marcelofabri marcelofabri left a comment

Choose a reason for hiding this comment

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

Can we have tests to cover this case? 🤔

@jpsim
Copy link
Collaborator Author

jpsim commented Jul 6, 2017

I actually think this fix is wrong, or at least incomplete. It won't work if the specified config isn't in the root path (e.g. swiftlint lint --config src/.swiftlint-ci.yml).

@marcelofabri
Copy link
Collaborator

@jpsim I think you're right (see #1631 and the fix in #1644)

alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request May 7, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request May 7, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request May 7, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request May 7, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request May 7, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request Sep 19, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request Sep 20, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request Sep 20, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
alvarhansen added a commit to alvarhansen/SwiftLint that referenced this pull request Sep 20, 2019
Removes configuration “working directory” check as the configuration
itself can reside in different directory. This check was introduced
with PR realm#1604 for issue realm#1531. At the time it was necessary as
SwiftLint did not support Configuration merging and this stopped
overwriting the configuration. Replaced it with configuration file
path check.

At the moment, if you load configuration from different path than root
path **and** you also have configuration file in root path then it is
skipped. Now that configuration merging is supported, it is more
correct to merge them.

Example where `Level3/.swiftlint.yml` is skipped:
`swiftlint lint --config ../.swiftlint.yml --path Level3`

```
Level2
│   .swiftlint.yml
│
└───Level3
│   │   .swiftlint.yml
│   │   Level3.swift
```

Adds test for making sure that configuration that is out of working
directory is merged with configuration from working directory.

Adds test for realm#1531 to make sure regression won’t happen in the future.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants