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

use trim_(start/end) insted of deprecated trim_(left/right) #1348

Closed
wants to merge 2 commits into from

Conversation

meltinglava
Copy link

No description provided.

@meltinglava
Copy link
Author

Tests fail on 1.27 since trim_start was in stable in 1.30. Do not know why the nightly failed. Nightly failed for some build error. No idea what. Looks like AppVeyor could not merge from forks other than master. Might be a config problem.

@rivy
Copy link
Member

rivy commented Apr 7, 2019

trim_start() wasn't added until v1.30.0; trim_left() is deprecated v1.33.0.

So, if this, or something similar is used, the minimum rust version would have to be pushed up to v1.30.

In my local 'dev' fork, to suppress spurious compilation warnings, I marked all the locations with #[allow(deprecated)] and a MAINT comment (see 9cc9cf7). I'm not sure if there's a better solution, so I didn't push it up as a PR. But, I think it's either that, or push the minimum version to v1.30.

@meltinglava
Copy link
Author

Indeed it is needed to bump minimum to 1.30. think this should be merged when/if another PR / commit forces the bump to >=1.30.

@Arcterus
Copy link
Collaborator

Arcterus commented Apr 23, 2019

Can you rebase? I am fine with updating the minimum version required. Probably we'd go to Rust 1.31 so we can use 2018 edition features.

@meltinglava
Copy link
Author

hope i did this right.

@meltinglava
Copy link
Author

meltinglava commented May 19, 2019

So i do not know why appveyor failed. Sry for takeing so long to respond

@Arcterus
Copy link
Collaborator

It looks like Appveyor just messed up again... It's not your fault. Also don't worry about it, I took a long time to respond before as well.

@Arcterus
Copy link
Collaborator

Arcterus commented May 19, 2019

It also looks like you merged instead of rebasing. If you don't know how to rebase, I think this page should help. If you're still unsure, let me know and I'll try to walk you through it.

@meltinglava
Copy link
Author

think i the rebase is right now. Please tell if it is not. Exams got in the way for doing it earlyer

@rivy
Copy link
Member

rivy commented Apr 14, 2020

Thanks for the contribution!
The fix for this same issue was already included in a large lint and CICD fixup PR (#1449).
We'd be happy to see you back with new contributions in the future.

@rivy rivy closed this Apr 14, 2020
@meltinglava meltinglava deleted the deprecated_stuff branch December 6, 2021 10:38
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.

3 participants