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

Remove .editorconfig #1696

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

flevi29
Copy link
Collaborator

@flevi29 flevi29 commented Aug 21, 2024

Pull Request

Why?

.editorconfig was added on the very first commit of this repository, but since then prettier has replaced it. Except it wasn't removed. There's no point in having multiple configurations for formatting. Related #1659, in which I wanted to remove all custom options, but didn't know why things still didn't work as they should, and now I know that prettier also takes .editorconfig into consideration.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@flevi29
Copy link
Collaborator Author

flevi29 commented Aug 21, 2024

I am not applying the fmt:fix, because it will be a lot of changes, for now. Once this is approved I can apply it, or better yet a member should apply it, so that they can trust that no malicious code is hidden in the changes and they don't even have to review the changes that way.

@flevi29 flevi29 requested a review from mdubus August 21, 2024 07:49
@flevi29 flevi29 added the maintenance Issue about maintenance (CI, tests, refacto...) label Aug 21, 2024
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 95.87629% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.11%. Comparing base (2d90d98) to head (a3b1fba).

Files with missing lines Patch % Lines
src/indexes.ts 85.71% 2 Missing ⚠️
src/http-requests.ts 93.33% 1 Missing ⚠️
tests/utils/meilisearch-test-utils.ts 93.75% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1696   +/-   ##
=======================================
  Coverage   97.11%   97.11%           
=======================================
  Files          21       21           
  Lines         831      831           
  Branches       77       77           
=======================================
  Hits          807      807           
- Misses         23       24    +1     
+ Partials        1        0    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@curquiza curquiza requested review from brunoocasali and removed request for mdubus August 21, 2024 09:39
curquiza
curquiza previously approved these changes Aug 21, 2024
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

I approve, but indeed the tests should be fixed before merging 😇

@curquiza curquiza removed the request for review from brunoocasali August 21, 2024 09:41
@flevi29
Copy link
Collaborator Author

flevi29 commented Aug 21, 2024

I approve, but indeed the tests should be fixed before merging 😇

Should I do it? It will be a lot of changes, or so I presume.
EDIT: I ran it

@curquiza
Copy link
Member

Thank you @flevi29
Same, we don't merge this PR until the coming release on Monday. I don't want to block: #1682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue about maintenance (CI, tests, refacto...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants