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

Clippy fixes. #846

Merged
merged 15 commits into from
Dec 15, 2021
Merged

Clippy fixes. #846

merged 15 commits into from
Dec 15, 2021

Conversation

Narsil
Copy link
Collaborator

@Narsil Narsil commented Dec 14, 2021

Fixing all new clippy warnings.

Copy link
Member

@McPatate McPatate left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -11,7 +11,7 @@ pub enum EncodeTask<'s> {
}

pub enum EncodeOutput {
Single(Encoding),
Single(Box<Encoding>),
Copy link
Member

Choose a reason for hiding this comment

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

Is Box needed because Javascript? 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, clippy yells becuse that variant is 240 bytes long (pretty long for an enum). Setting it as a box just uses a pointer instead.

"neon-cli": "^0.3.3",
"neon-cli": "^0.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

Big jump! 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

npm audit --fix did it !

@Narsil
Copy link
Collaborator Author

Narsil commented Dec 15, 2021

I will merge this so we can rebase other PRs.

@n1t0 we can still discuss some choices here.

  • 3.6 MacOS as a target was dropped by GA (so we cannot run it)
  • Had to modify the testing because MacOS Python 3.8+ uses spawn and not fork anymore. (Just modifying the tests seems reasonable)
  • Had to bump setup-python@v2 because otherwise the lib location was not found by rust.
  • Had to npm audit --fix (security warning for tmpl)
  • pyarrow < 3.0 was invalid since now datasets requires >= 3.0 and the original reason for downgrading seems gone. (We also were using the pip version that tried EVERY version to try and find version resolution, which lead to the job running for 5h...., now solved by adding pip install -U pip)

@Narsil Narsil merged commit c1100ec into master Dec 15, 2021
@Narsil Narsil deleted the fix_clippy branch December 15, 2021 14:56
McPatate pushed a commit that referenced this pull request Dec 15, 2021
* Clippy fixes.

* Drop support for Python 3.6

* Remove other 3.6

* Re-enabling caches for build (5h + seems too long and issue seems
solved)

actions/runner-images#572

* `npm audit fix`.

* Fix yaml ?

* Pyarrow issue fixed: huggingface/datasets#2268

* Installing dev libraries.

* Install python dev elsewhere ?

* Typo.

* No sudo.

* ...

* Testing the GH again.

* Maybe v2 will fix ?

* Fixing tests on MacOS Python 3.8+
McPatate pushed a commit that referenced this pull request Dec 22, 2021
* Clippy fixes.

* Drop support for Python 3.6

* Remove other 3.6

* Re-enabling caches for build (5h + seems too long and issue seems
solved)

actions/runner-images#572

* `npm audit fix`.

* Fix yaml ?

* Pyarrow issue fixed: huggingface/datasets#2268

* Installing dev libraries.

* Install python dev elsewhere ?

* Typo.

* No sudo.

* ...

* Testing the GH again.

* Maybe v2 will fix ?

* Fixing tests on MacOS Python 3.8+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants