Combine apt, pacman & dnf block to packages block

[IshanGrover2004]

Worked on idea proposed in Issue #1964

Before

Individual blocks for checking updates

[[block]]
block = "apt"
interval = 1800
format = "$icon $count updates available"
format_singular = " $icon One update available "
format_up_to_date = " $icon system up to date "

After

Unified apt, pacman, aur, dnf blocks in packages block and deprecates the old block

[[block]]
block = "packages"
package_manager = ["apt", "pacman", "aur","dnf"]
interval = 1800
format = "$icon $apt + $pacman + $aur + $dnf = $total updates available"
format_singular = " $icon One update available "
format_up_to_date = " $icon system up to date "

I have changed the structure of the code by adding a Backend trait which we can implement in all struct for every package manager. I did try to keep the code very generic which made easy to add any package manager in future

closes #1958

[MaxVerevkin]

Let's not delete the old blocks for now, just deprecate.

[IshanGrover2004]

Let's not delete the old blocks for now, just deprecate.

Done @MaxVerevkin

Any other thing to do?

[bim9262]

It also seems that there's a lot of duplicate code between these and the deprecated blocks (which I don't see noted anywhere). I'd try to reuse this new code by importing from these new modules. That way if there's a bugfix/improvement made to one block it's made for both (although it may not be worth the effort depending on how long we plan to do the deprecation for).

[bim9262]

I'd suggest the following changes to note the deprecated blocks (but maybe @MaxVerevkin has some other recommendations. I can open this in another PR if you want this to rebase on the change)

diff --git a/src/blocks.rs b/src/blocks.rs
index 836e65f7..4af7e6a5 100644
--- a/src/blocks.rs
+++ b/src/blocks.rs
@@ -45,11 +45,16 @@ use crate::{BoxedFuture, Request, RequestCmd};
 
 macro_rules! define_blocks {
     {
-        $( $(#[cfg(feature = $feat: literal)])? $block: ident $(,)? )*
+        $(
+            $(#[cfg(feature = $feat: literal)])?
+            $(#[deprecated($($dep_k: ident = $dep_v: literal),+)])?
+            $block: ident $(,)?
+        )*
     } => {
         $(
             $(#[cfg(feature = $feat)])?
             $(#[cfg_attr(docsrs, doc(cfg(feature = $feat)))])?
+            $(#[deprecated($($dep_k = $dep_v),+)])?
             pub mod $block;
         )*
 
@@ -58,6 +63,7 @@ macro_rules! define_blocks {
             $(
                 $(#[cfg(feature = $feat)])?
                 #[allow(non_camel_case_types)]
+                #[allow(deprecated)]
                 $block($block::Config),
             )*
             Err(&'static str, Error),
@@ -68,6 +74,7 @@ macro_rules! define_blocks {
                 match self {
                     $(
                         $(#[cfg(feature = $feat)])?
+                        #[allow(deprecated)]
                         Self::$block { .. } => stringify!($block),
                     )*
                     Self::Err(name, _err) => name,
@@ -78,6 +85,7 @@ macro_rules! define_blocks {
                 match self {
                     $(
                         $(#[cfg(feature = $feat)])?
+                        #[allow(deprecated)]
                         Self::$block(config) => futures.push(async move {
                             while let Err(err) = $block::run(&config, &api).await {
                                 if api.set_error(err).is_err() {
@@ -114,6 +122,7 @@ macro_rules! define_blocks {
                 match block_name {
                     $(
                         $(#[cfg(feature = $feat)])?
+                        #[allow(deprecated)]
                         stringify!($block) => match $block::Config::deserialize(table) {
                             Ok(config) => Ok(BlockConfig::$block(config)),
                             Err(err) => Ok(BlockConfig::Err(stringify!($block), crate::errors::Error::new(err.to_string()))),
@@ -136,6 +145,10 @@ macro_rules! define_blocks {
 
 define_blocks!(
     amd_gpu,
+    #[deprecated(
+        since = "0.32.4",
+        note = "The block has been deprecated in favor of the the packages block"
+    )]
     apt,
     backlight,
     battery,
@@ -163,6 +176,10 @@ define_blocks!(
     notmuch,
     nvidia_gpu,
     packages,
+    #[deprecated(
+        since = "0.32.4",
+        note = "The block has been deprecated in favor of the the packages block"
+    )]
     pacman,
     pomodoro,
     rofication,

EDIT: format fix

[IshanGrover2004]

diff --git a/src/blocks.rs b/src/blocks.rs

---

• $( $(#[cfg(feature = $feat: literal)])? $block: ident $(,)? )*

• $( $(#[cfg(feature = $feat: literal)])?
• $(#[deprecated($($dep_k: ident = $dep_v: literal),+)])?
• $block: ident $(,)? )*

---

Do I need to do these changes and send commit? or maybe wait for @MaxVerevkin 's reply

[bim9262]

I'd wait to see if @MaxVerevkin has any feedback/suggestions first

[MaxVerevkin]

Deprecation patch looks good to me. Just note that the next version is 0.33. And hopefully in the future we will not need to annotate everything with #[allow(deprecated)] as apparently warnings inside the defining crate are a bug - rust-lang/rust#47219. By the way, allow(deprecated) in BlockConfig::name() is not necessary.

[IshanGrover2004]

I did the changes proposed
Anyother work todo here?

[IshanGrover2004]

Any other thing left to change @MaxVerevkin

[IshanGrover2004]

This PR is ready to merge i guess since no more review left @MaxVerevkin

[c3Ls1US]

Hey, great work for this feature and I agree with unifying the package manager blocks.

Though, have you already explored using libalpm for pacman's database functionality? I believe there are Rust bindings for it and am just curious why you decided not to go this way.

[MaxVerevkin]

Please add a deprecation warning for dnf too.

[IshanGrover2004]

Please add a deprecation warning for dnf too.

Done ✔️

[MaxVerevkin]

LGTM!

Just some recommendations regarding the docs:

• The example section may be made a bit less verbose, for example by not including critical_updates_regex in every example.
• "Default" column for package_manager should mention that it is automatically derived from the format templates, but can be used to influence the $total value.

[IshanGrover2004]

hey @MaxVerevkin ,
Do the $both works here if $both value is not set in code in block/packages

format = " $icon $pacman + $aur = $both updates available "

If not work, should i remove $both from here

[MaxVerevkin]

hey @MaxVerevkin ,
Do the $both works here if $both value is not set in code in block/packages

format = " $icon $pacman + $aur = $both updates available "

If not work, should i remove $both from here

Oh, yes, missed this one. This should be $total instead of $both.

[IshanGrover2004]

Oh, yes, missed this one. This should be $total instead of $both.

Done ✔️

[MaxVerevkin]

Thanks!