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

simonsan · 2023-12-20T00:28:56Z

Tests run through, the benchmark was broken though native_db::Database::create_in_memory() was not existing.

Signed-off-by: simonsan <[email protected]>

src/watch/mod.rs

vincent-herlemont · 2023-12-20T07:53:09Z

.github/workflows/ci.yml

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

Integrate it into the existing file .github/workflows/build_and_test_release.yml

Or rename it and remove the 'test' part, as this is already done in this workflow .github/workflows/build_and_test_release.yml

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?

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!

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.).

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

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:

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:

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.

@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.

vincent-herlemont · 2023-12-20T07:55:30Z

native_db_macro/src/keys.rs

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


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

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?

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

vincent-herlemont · 2023-12-20T07:58:26Z

src/db_type/key/inner_key_value.rs

-        match self {
-            Some(value) => value.database_inner_key_value(),
-            None => DatabaseInnerKeyValue::new(Vec::new()),
-        }


Is it possible to keep the match notation? I find it more readable.
Unless there's another interest in using the map_or_else method?

It's less idiomatic, but it is also personal preference. I personally find it more readable with the map, as it makes it more explicit, that we operate on self.

Follow up #64 (comment)

vincent-herlemont · 2023-12-20T07:59:09Z

src/db_type/output.rs

-pub(crate) fn unwrap_item<T: Input>(item: Option<redb::AccessGuard<&'static [u8]>>) -> Option<T> {
-    if let Some(item) = item {
-        let item = item.value();
-        let item = T::native_db_bincode_decode_from_slice(item);
-        Some(item)
-    } else {
-        None
-    }


Is it possible to keep the match notation? I find it more readable.
Unless there's another interest in using the map_or_else method?

It's less idiomatic, I would say. As essentially you are destructing an Option to then return an Option. It would be different, if we would return item. If you are working with Option and returning Option, I find it more idiomatic, work within the option.

It's less idiomatic, but it is also personal preference. I personally find it more readable with the map, as it makes it more explicit, that we operate on self.
It's less idiomatic, I would say. As essentially you are destructing an Option to then return an Option. It would be different, if we would return item. If you are working with Option and returning Option, I find it more idiomatic, work within the option.

Maybe you are making me change my mind. I understand your arguments are relevant. I just have a problem with the aesthetics/visibility/symmetry of the code...

vincent-herlemont · 2023-12-20T07:59:45Z

src/table_definition.rs

-impl Default for NativeModelOptions {
-    fn default() -> Self {
-        Self {
-            native_model_id: 0,
-            native_model_version: 0,
-            native_model_legacy: false,
-        }
-    }
-}
-


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

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

Follow up #64 (comment)

src/watch/mod.rs

vincent-herlemont · 2023-12-20T08:05:04Z

@simonsan Thank you very much for your merge request!

vincent-herlemont · 2024-05-01T12:31:57Z

All is done now.

simonsan added 2 commits December 20, 2023 01:27

chore: update to 2021 edition and implement clippy fixes

7c70779

Signed-off-by: simonsan <[email protected]>

ci: add ci

f12f9ae

Signed-off-by: simonsan <[email protected]>

simonsan commented Dec 20, 2023

View reviewed changes

src/watch/mod.rs Show resolved Hide resolved

vincent-herlemont enabled auto-merge (rebase) December 20, 2023 06:12

vincent-herlemont disabled auto-merge December 20, 2023 06:12

vincent-herlemont reviewed Dec 20, 2023

View reviewed changes

vincent-herlemont closed this May 1, 2024

simonsan deleted the clippy-fixes branch May 1, 2024 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

simonsan commented Dec 20, 2023 •

edited

Loading

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

simonsan Dec 20, 2023 •

edited

Loading

vincent-herlemont Dec 23, 2023

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

vincent-herlemont Dec 20, 2023

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

vincent-herlemont Dec 20, 2023

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

vincent-herlemont Dec 20, 2023

vincent-herlemont Dec 20, 2023

simonsan Dec 20, 2023

vincent-herlemont Dec 20, 2023

vincent-herlemont commented Dec 20, 2023

vincent-herlemont commented May 1, 2024

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

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

Conversation

simonsan commented Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonsan Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincent-herlemont commented Dec 20, 2023

vincent-herlemont commented May 1, 2024

simonsan commented Dec 20, 2023 •

edited

Loading

simonsan Dec 20, 2023 •

edited

Loading