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

Lints #116

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Lints #116

merged 3 commits into from
Nov 21, 2023

Conversation

Grinkers
Copy link
Collaborator

Added lints future-incompatible and rust_2018_idioms and appropriate changes.

https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html#lint-configuration-through-cargo
Now using cargo's lint configuration from 1.74. In the near future, the CI can also be changed to

- run: cargo clippy --all-features -- --deny warnings
- run: cargo clippy --no-default-features -- --deny warnings

…egration.

Added rust's base `future-incompatible` and `rust_2018_idioms` to CI (although redundant from 1.74).
Lint fixes for tests (now exposed via cargo from rust 1.74).
@Grinkers Grinkers marked this pull request as ready for review November 20, 2023 14:56
@Grinkers Grinkers merged commit fe6b043 into naomijub:master Nov 21, 2023
@Grinkers Grinkers deleted the lints branch November 21, 2023 07:33
@Grinkers
Copy link
Collaborator Author

@naomijub please delete branch https://github.com/naomijub/edn-rs/tree/lints this is the correct/cleaned version

@evaporei
Copy link
Collaborator

evaporei commented Nov 21, 2023

I tried to, but @naomijub has to do it, I don't have access to the repo Settings page 😕
2023-11-21_08-57

@naomijub
Copy link
Owner

naomijub commented Nov 21, 2023

WOW, I can't do it as well. Let me check if there is a way to delete it

EDIT:
There was a weird ruleset on deletion

@Grinkers
Copy link
Collaborator Author

Grinkers commented Nov 22, 2023

I think it'd be nice to remove those rulesets on non-master branches (which allow for cleaning up commits, quick mistakes like this missing async, etc). In particular allowing --force. I don't really mind doing my messy --force elsewhere too. The most strict rules are great for master.

Speaking of me missing async @naomijub what was the purpose of the async feature? I keep forgetting to check it because it's not really on my mind. I'm either missing something or maybe it's a legacy thing from pre-2018? I know rust async has gone through a rough road. From my understanding, there's no point in making anything in this library async because serialization and deserialization require all data to be fully awaited already. There's no support for incrementally loading of partial bytes as they come in or go out, nor does that really make much sense.

Just as an example, off master right now

Cargo.toml

edition = "2021"

[dependencies]
edn-rs = { git = "https://github.com/Grinkers/edn-rs.git", default-features = false }
tokio = { version = "1", features = ["full"] }

main.rs

use std::str::{self, FromStr};

use edn_rs::{edn, Edn, Vector};
use tokio::fs::File;
use tokio::io::AsyncReadExt;

#[tokio::main]
async fn main() -> std::io::Result<()> {
    let mut file = File::open("foo.txt").await?; // assume the file exists and has valid edn
    let mut contents = vec![];
    file.read_to_end(&mut contents).await?;

    let edn = Edn::from_str(str::from_utf8(&contents).unwrap());
    println!("{edn:?}");

    let edn = edn!([1 1.5 "hello" :key]);
    println!("{edn:?}");
    Ok(())
}

fully works as expected, without the feature.

@naomijub
Copy link
Owner

I removed the delete rule.

Async feature was created before async await was fully stable, so I guess now they are useless

@Grinkers
Copy link
Collaborator Author

I will clean it up, depending on what we do with #117

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