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

Fix all swiftlint errors and remove Tests from the linter #134

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

ppamorim
Copy link
Contributor

@ppamorim ppamorim commented May 19, 2021

Hi,

In this PR I fixed all swiftlint errors and removed the Tests and Demos folder for the linter, this sorts the issue #97.
I did fix the errors in points of the code that are going to be removed, as listed on #133.

My local linter is on the latest version 0.42.0.

Multiple linter fixes applied by running swiftlint autocorrect.

Regards.

@ppamorim ppamorim added the enhancement New feature or request label May 19, 2021
@ppamorim ppamorim requested review from curquiza and bidoubiwa May 19, 2021 21:54
@ppamorim ppamorim linked an issue May 19, 2021 that may be closed by this pull request
@ppamorim ppamorim force-pushed the feature/swiftlintIgnoreTests branch from fb91ae9 to 88565ba Compare May 19, 2021 22:59
@ppamorim
Copy link
Contributor Author

Any idea why the checks are being cancelled? Internal error?

@ppamorim
Copy link
Contributor Author

@curquiza I had to update Xcode to 12.4 to support decode(String?.self, ...) otherwise it crashes on Xcode 11.5.

- PerfectDemo/.build
- Demos/VaporDemo
- Demos/PerfectDemo
- Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we exclude these folders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need swiftlint in the demos and tests folder at the moment. I can open a new PR to sort them all later and reintroduce them back to the swiftlint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, really sorry @ppamorim but linting is bound to a project as a whole not only the source code. It creates consistency and makes contributing easier.
It would be weird if I don't have to follow the same rules in the tests folder than in the source folder.

Sources/MeiliSearch/Config.swift Outdated Show resolved Hide resolved
@bidoubiwa
Copy link
Contributor

Thanks a lot for this PR! Sorry for the delay

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
@ppamorim ppamorim requested a review from bidoubiwa May 31, 2021 20:57
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

per the discussion we have in a thread.

For consistency in coding practices, I prefer linting to be applied in tests and demo's dir's as well.

#137

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM

@bidoubiwa
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 8, 2021

Build succeeded:

@bors bors bot merged commit b773f1c into main Jun 8, 2021
@bors bors bot deleted the feature/swiftlintIgnoreTests branch June 8, 2021 12:05
@curquiza curquiza added skip-changelog The PR will not appear in the release changelogs and removed enhancement New feature or request labels Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog The PR will not appear in the release changelogs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove linter warnings
3 participants