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

Add missing packages for yarn lint and upgrade all #123

Merged
merged 5 commits into from
Nov 9, 2020

Conversation

bb
Copy link
Contributor

@bb bb commented Nov 5, 2020

Add packages for yarn lint and upgrade all packages. Might be fine to merge, if it doesn't break anything for you. It wasn't working for me before the changes either, so I can't test. See #124 for more information about my issues.

@bb
Copy link
Contributor Author

bb commented Nov 5, 2020

As most times, the issue was quite obvious in hindsight. Lint was checking all files in node_modules and dist (also those in sub folders like examples/esm) because they were not ignored. The second commit improves that and thus also fixes #124.

@bb bb mentioned this pull request Nov 5, 2020
@bb
Copy link
Contributor Author

bb commented Nov 5, 2020

I added another commit which adds missing peer (dev-) dependencies: @babel/core and typescript.

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.

Awesome!

Could you add a CI that tests this? You can change .github/workflows/test.yml and add the following job:

jobs: 
    style: instant-meilisearch-style-tests
        name: nstant-meilisearch-style-tests
        runs-on: ubuntu-latest
        name: Tests style
        run: yarn lint

and inside ./bors.toml
Change this value:

status = ["instant-meilisearch-tests"]

To this one:

status = ["instant-meilisearch-tests", "instant-meilisearch-style-tests"]

@bb
Copy link
Contributor Author

bb commented Nov 9, 2020

bors try

@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

🔒 Permission denied

Existing reviewers: click here to make bb a reviewer

@bb
Copy link
Contributor Author

bb commented Nov 9, 2020

@bidoubiwa I added the changes you requested (with slight syntax changes and added the yarn install step) in the text.yml but it doesn't look like it's running in the branch. I tried to trigger it but got permission denied. Can you please check?

@bidoubiwa
Copy link
Contributor

bors try

@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

try

Merge conflict.

@bidoubiwa
Copy link
Contributor

It seems to be failing because of a merge conflict. Could you rebase?

curquiza
curquiza previously approved these changes Nov 9, 2020
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.

Your PR is really great @bb! 😁
The merge conflict is indeed due to all the package updates you've done in this PR.

As @bidoubiwa said, you need to rebase and fix the merge conflict at the same time. I don’t know if you are already on our Slack, feel free to joins us, it would be easier to discuss if you need help with the rebase (I’m Clementine). And we also have a #contributions channel if you are interested in MeiliSearch’s tools contributions in general!

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.

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

@bors bors bot merged commit 458f4ed into meilisearch:master Nov 9, 2020
@bb bb deleted the missing-lint-packages branch November 9, 2020 14:49
@curquiza curquiza added the skip-changelog The PR will not appear in the release changelogs label Nov 10, 2020
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.

3 participants