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.

Sign up for GitHub

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

Jump to bottom

Update to 2021 edition, add ci + clippy fixes #64

Closed
wants to merge 2 commits into from
Closed

Update to 2021 edition, add ci + clippy fixes #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c70779
chore: update to 2021 edition and implement clippy fixes
simonsan Dec 20, 2023
f12f9ae
ci: add ci
simonsan Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 155 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Copy link
Owner

Choose a reason for hiding this comment

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

Nice addition of this workflow file. I see two solutions:

Copy link
Author

Choose a reason for hiding this comment

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

This workflow has the advantage, that it will work with the merge queue. In general, the actions in build_and_test_release.yml are outdated and need a rework, unfortunately. actions-rs as an org is archived, meaning all the actions of them are unmaintained. Maybe we can keep the ci.yml so there are some checks in PRs for now until the build_and_test_release.yml is updated?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I know it's no longer maintained, but build_and_test_release.yml allows testing on all platforms, manages releases, etc., while ci.yml does not. It would be wiser to update build_and_test_release.yml and separate these changes into different MRs:

  • One PR for upgrading build_and_test_release.yml tests workflow.
  • One PR to add a job for clippy.
  • One PR for upgrading to 2021.
  • One PR for CI docs.
  • One PR for powerset / cargo hack check.

All these modifications are good to go. I suggest closing this MR and, if you wish, opening specific ones, which I would greatly appreciate :). In any case, it's a great idea, and even if it's not essential, it will be done in the coming weeks!

Copy link
Author

Choose a reason for hiding this comment

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

2023-12-20 20_39_03-rustic_ github_workflows at main · rustic-rs_rustic — Mozilla Firefox

src.: https://github.com/rustic-rs/rustic/tree/main/.github/workflows

I would recommend separating the different workflows by their scope and when they are run. Basic checking and testing, fmt, etc. for PRs. Releasing and running time and resource intense workflows on user request (see prebuilt-pr above) because some resource intense stuff you don't want to run on PRs (environmental impact, not useful, etc.).

Copy link
Author

@simonsan simonsan Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That being said I think build or rather check and test should not be in the same job:

2023-12-20 22_51_05-Update to 2021 edition, add ci + clippy fixes · vincent-herlemont_native_db@f12f

Because one is not dependent on the other, in terms of CI. You want to have output for check/build and a separate output (checkmark) for tests. The more fine-grained the results are, the more useful overall. Otherwise, if that workflow fails, you always need to check, which step of that workflow failed, whereas one job should focus on one thing. And reduce the noise as much as possible.

The current workflow:
2023-12-20 22_59_40-Update to 2021 edition, add ci + clippy fixes · vincent-herlemont_native_db@f12f

Is kind of os specific and doesn't give a lot of information what exactly failed and where the probably lay.

While e.g. the proposed workflow gives you:
2023-12-20 22_57_36-Update to 2021 edition, add ci + clippy fixes · vincent-herlemont_native_db@f12f

An overview of what is happening and doesn't necessarily put os-dependent problems up front, while you still can investigate them, because they are just a build matrix.

Copy link
Owner

Choose a reason for hiding this comment

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

@simonsan Thank you for your explanations. I have started to refactor the CI on main. I've made separate workflows, and the next step will be to remove the deprecated jobs, and split in test, build etc. Please don't hesitate to give me your opinion; it is valuable.

Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
name: Continuous Integration

on:
pull_request:
paths-ignore:
- "**/*.md"
push:
branches:
- main
paths-ignore:
- "**/*.md"
schedule:
- cron: "0 0 * * 0"
merge_group:
types: [checks_requested]

jobs:
fmt:
name: Rustfmt
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 # v1
with:
toolchain: stable
- run: rustup component add rustfmt
- name: Run Cargo Fmt
run: cargo fmt --all -- --check

clippy:
name: Clippy
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 # v1
with:
toolchain: stable
components: clippy
- uses: Swatinem/rust-cache@a95ba195448af2da9b00fb742d14ffaaf3c21f43 # v2
- name: Run clippy
run: cargo clippy --all-targets --all-features -- -D warnings

test:
name: Test
runs-on: ${{ matrix.job.os }}
strategy:
matrix:
rust: [stable]
job:
- os: macos-latest
- os: ubuntu-latest
- os: windows-latest
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name != 'pull_request'
with:
fetch-depth: 0

- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name == 'pull_request'
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 # v1
with:
toolchain: ${{ matrix.rust }}
- uses: Swatinem/rust-cache@a95ba195448af2da9b00fb742d14ffaaf3c21f43 # v2
- name: Run Cargo Test
run: cargo +${{ matrix.rust }} test -r --all-targets --all-features --workspace

docs:
name: Build docs
runs-on: ${{ matrix.job.os }}
strategy:
matrix:
rust: [stable]
job:
- os: macos-latest
- os: ubuntu-latest
- os: windows-latest
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name != 'pull_request'
with:
fetch-depth: 0

- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name == 'pull_request'
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 # v1
with:
toolchain: ${{ matrix.rust }}
- uses: Swatinem/rust-cache@a95ba195448af2da9b00fb742d14ffaaf3c21f43 # v2
- name: Run Cargo Doc
run: cargo +${{ matrix.rust }} doc --no-deps --all-features --workspace --examples

powerset:
name: Check Powerset of Features
runs-on: ${{ matrix.job.os }}
strategy:
matrix:
rust: [stable, beta, nightly]
job:
- os: macos-latest
- os: ubuntu-latest
- os: windows-latest
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name != 'pull_request'
with:
fetch-depth: 0

- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
if: github.event_name == 'pull_request'
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 # v1
with:
toolchain: ${{ matrix.rust }}
- name: install cargo-hack
uses: taiki-e/install-action@2b8d4e021f3c5d9b9d4726c038ac367d3ed730b6 # v2
with:
tool: cargo-hack
- uses: Swatinem/rust-cache@a95ba195448af2da9b00fb742d14ffaaf3c21f43 # v2
- name: Run Cargo Hack
run: cargo +${{ matrix.rust }} hack check --feature-powerset --no-dev-deps

result:
name: Result (CI)
runs-on: ubuntu-latest
needs:
- fmt
- clippy
- test
- docs
- powerset
steps:
- name: Mark the job as successful
run: exit 0
if: success()
- name: Mark the job as unsuccessful
run: exit 1
if: "!success()"
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "native_db"
version = "0.5.1"
authors = ["Vincent Herlemont <[email protected]>"]
edition = "2018"
edition = "2021"
description = "Drop-in embedded database"
license = "MIT"
repository = "https://github.com/vincent-herlemont/native_db"
Expand Down
2 changes: 1 addition & 1 deletion build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ fn main() {
// Run skeptic
skeptic::generate_doc_tests(&["README.md"]);
}
}
}
11 changes: 1 addition & 10 deletions native_db_macro/src/keys.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl<O: ToTokenStream> ToTokenStream for DatabaseKeyDefinition<O> {
}
}

#[derive(Clone)]
#[derive(Clone, Default)]
pub(crate) struct DatabaseSecondaryKeyOptions {
pub(crate) unique: bool,
pub(crate) optional: bool,
Expand All @@ -62,15 +62,6 @@ impl ToTokenStream for () {
}
}

impl Default for DatabaseSecondaryKeyOptions {
fn default() -> Self {
Self {
unique: false,
optional: false,
}
}
}

Comment on lines -65 to -73
Copy link
Owner

Choose a reason for hiding this comment

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

For this part, is it possible to keep the default explicit? Since these are options, it has real logical significance and implications.

Copy link
Author

Choose a reason for hiding this comment

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

Can do, but do you want to keep false or replace it with bool::default() to make it more explicit, that default values are being used?

Copy link
Owner

Choose a reason for hiding this comment

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

No, it's fine, let's leave it as it is. There isn't too much ambiguity at that level ^^

impl<O: ToTokenStream> DatabaseKeyDefinition<O> {
pub(crate) fn name(&self) -> String {
if let Some(field_name) = &self.field_name {
Expand Down
8 changes: 4 additions & 4 deletions native_db_macro/src/model_attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl ModelAttributes {
} else {
panic!(
"Unknown attribute: {}",
meta.path.get_ident().unwrap().to_string()
meta.path.get_ident().unwrap()
);
}
Ok(())
Expand All @@ -46,7 +46,7 @@ impl ModelAttributes {
} else {
panic!(
"Unknown attribute: {}",
meta.path.get_ident().unwrap().to_string()
meta.path.get_ident().unwrap()
);
}
Ok(())
Expand All @@ -55,7 +55,7 @@ impl ModelAttributes {
} else {
panic!(
"Unknown attribute: {}",
meta.path.get_ident().unwrap().to_string()
meta.path.get_ident().unwrap()
);
}
Ok(())
Expand All @@ -71,7 +71,7 @@ impl ModelAttributes {
));
} else if attr.path().is_ident("secondary_key") {
let mut secondary_options = DatabaseSecondaryKeyOptions::default();
if let Ok(_) = attr.meta.require_list() {
if attr.meta.require_list().is_ok() {
attr.parse_nested_meta(|meta| {
if meta.path.is_ident("unique") {
secondary_options.unique = true;
Expand Down
29 changes: 12 additions & 17 deletions src/database.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::sync::atomic::AtomicU64;
use std::sync::{Arc, RwLock};
use std::u64;

/// The database instance. Allows you to create [rw_transaction](database/struct.Database.html#method.rw_transaction) and [r_transaction](database/struct.Database.html#method.r_transaction), [watch](database/struct.Database.html#method.watch) queries, and [unwatch](database/struct.Database.html#method.unwatch) etc.
/// The database instance. Allows you to create [`rw_transaction`](database/struct.Database.html#method.rw_transaction) and [`r_transaction`](database/struct.Database.html#method.r_transaction), [watch](database/struct.Database.html#method.watch) queries, and [unwatch](database/struct.Database.html#method.unwatch) etc.
///
/// # Example
/// ```rust
Expand All @@ -40,7 +40,7 @@ pub struct Database<'a> {

impl Database<'_> {
/// Creates a new read-write transaction.
pub fn rw_transaction(&self) -> Result<RwTransaction> {
pub fn rw_transaction(&self) -> Result<RwTransaction<'_>> {
let rw = self.instance.begin_write()?;
let write_txn = RwTransaction {
watcher: &self.watchers,
Expand All @@ -54,7 +54,7 @@ impl Database<'_> {
}

/// Creates a new read-only transaction.
pub fn r_transaction(&self) -> Result<RTransaction> {
pub fn r_transaction(&self) -> Result<RTransaction<'_>> {
let txn = self.instance.begin_read()?;
let read_txn = RTransaction {
internal: InternalRTransaction {
Expand All @@ -68,7 +68,7 @@ impl Database<'_> {

impl Database<'_> {
/// Watch queries.
pub fn watch(&self) -> Watch {
pub const fn watch(&self) -> Watch<'_> {
Watch {
internal: InternalWatch {
watchers: &self.watchers,
Expand All @@ -82,8 +82,7 @@ impl Database<'_> {
/// If the `id` is not valid anymore, this function will do nothing.
/// If the `id` is valid, the corresponding watcher will be removed.
pub fn unwatch(&self, id: u64) -> Result<()> {
let mut watchers = self.watchers.write().unwrap();
watchers.remove_sender(id);
self.watchers.write().unwrap().remove_sender(id);
Ok(())
}
}
Expand All @@ -92,26 +91,22 @@ impl<'a> Database<'a> {
pub(crate) fn seed_model(&mut self, model_builder: &'a ModelBuilder) -> Result<()> {
let main_table_definition =
redb::TableDefinition::new(model_builder.model.primary_key.unique_table_name.as_str());
let mut primary_table_definition: PrimaryTableDefinition =
let mut primary_table_definition: PrimaryTableDefinition<'_> =
(model_builder, main_table_definition).into();

let rw = self.instance.begin_write()?;
rw.open_table(primary_table_definition.redb.clone())?;
_ = rw.open_table(primary_table_definition.redb)?;

for secondary_key in model_builder.model.secondary_keys.iter() {
primary_table_definition.secondary_tables.insert(
_ = primary_table_definition.secondary_tables.insert(
secondary_key.clone(),
redb::TableDefinition::new(secondary_key.unique_table_name.as_str()).into(),
);
rw.open_table(
primary_table_definition.secondary_tables[&secondary_key]
.redb
.clone(),
)?;
_ = rw.open_table(primary_table_definition.secondary_tables[&secondary_key].redb)?;
}
rw.commit()?;

self.primary_table_definitions.insert(
_ = self.primary_table_definitions.insert(
model_builder.model.primary_key.unique_table_name.clone(),
primary_table_definition,
);
Expand All @@ -124,7 +119,7 @@ impl<'a> Database<'a> {
let rx = self.instance.begin_read()?;
let mut stats_primary_tables = vec![];
for primary_table in self.primary_table_definitions.values() {
let result_table_open = rx.open_table(primary_table.redb.clone());
let result_table_open = rx.open_table(primary_table.redb);
let stats_table = match result_table_open {
Err(redb::TableError::TableDoesNotExist(_)) => StatsTable {
name: primary_table.redb.name().to_string(),
Expand All @@ -146,7 +141,7 @@ impl<'a> Database<'a> {
let mut stats_secondary_tables = vec![];
for primary_table in self.primary_table_definitions.values() {
for secondary_table in primary_table.secondary_tables.values() {
let result_table_open = rx.open_table(secondary_table.redb.clone());
let result_table_open = rx.open_table(secondary_table.redb);
let stats_table = match result_table_open {
Err(redb::TableError::TableDoesNotExist(_)) => StatsTable {
name: secondary_table.redb.name().to_string(),
Expand Down
Loading
Loading