Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: simonsan <14062932+simonsan@users.noreply.github.com>
  • Loading branch information
aawsome and simonsan authored Aug 7, 2023
1 parent 19e5f01 commit 34972d3
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 29 deletions.
4 changes: 2 additions & 2 deletions crates/rustic_core/examples/merge.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! `merge` example
use rustic_core::{last_modified, repofile::SnapshotFile, Repository, RepositoryOptions};
use rustic_core::{last_modified_node, repofile::SnapshotFile, Repository, RepositoryOptions};
use simplelog::{Config, LevelFilter, SimpleLogger};
use std::error::Error;

Expand All @@ -16,7 +16,7 @@ fn main() -> Result<(), Box<dyn Error>> {
// Merge all snapshots using the latest entry for duplicate entries
let snaps = repo.get_all_snapshots()?;
// This creates a new snapshot without removing the used ones
let snap = repo.merge_snapshots(&snaps, &last_modified, SnapshotFile::default())?;
let snap = repo.merge_snapshots(&snaps, &last_modified_node, SnapshotFile::default())?;

println!("successfully created snapshot:\n{snap:#?}");
Ok(())
Expand Down
2 changes: 1 addition & 1 deletion crates/rustic_core/src/backend/ignore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::{

// Walk doesn't implement Debug
#[allow(missing_debug_implementations)]
/// A [`LocalSource`] is a source from local paths which is used to read from (i.e. to backup it).
/// A [`LocalSource`] is a source from local paths which is used to be read from (i.e. to backup it).
pub struct LocalSource {
builder: WalkBuilder,
walker: Walk,
Expand Down
15 changes: 8 additions & 7 deletions crates/rustic_core/src/backend/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,10 +267,11 @@ pub struct LocalDestination {
impl LocalDestination {
/// Create a new [`LocalDestination`]
///
/// Arguments:
/// - `path`: The base path of the destination
/// - `create`: If `create` is true, create the base path if it doesn't exist.
/// - `expect_file`: Whether we expect a single file as destination.
/// # Arguments
///
/// * `path` - The base path of the destination
/// * `create` - If `create` is true, create the base path if it doesn't exist.
/// * `expect_file` - Whether we expect a single file as destination.
pub fn new(path: &str, create: bool, expect_file: bool) -> RusticResult<Self> {
let is_dir = path.ends_with('/');
let path: PathBuf = path.into();
Expand Down Expand Up @@ -573,9 +574,9 @@ impl LocalDestination {
Ok(vec.into())
}

/// Check if amatching file exists.
/// Check if a matching file exists.
/// If a file exists and size matches, this returns a `File` open for reading.
/// In all other cases, retruns `None`
/// In all other cases, returns `None`
pub fn get_matching_file(&self, item: impl AsRef<Path>, size: u64) -> Option<File> {
let filename = self.path(item);
fs::symlink_metadata(&filename).map_or_else(
Expand All @@ -590,7 +591,7 @@ impl LocalDestination {
)
}

/// Write `data`to given item (relative to the base path) at `offset`
/// Write `data` to given item (relative to the base path) at `offset`
pub fn write_at(&self, item: impl AsRef<Path>, offset: u64, data: &[u8]) -> RusticResult<()> {
let filename = self.path(item);
let mut file = fs::OpenOptions::new()
Expand Down
26 changes: 16 additions & 10 deletions crates/rustic_core/src/backend/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ use crate::id::Id;
pub struct Node {
/// Name of the node: filename or dirname.
///
///Warning: This contains an escaped variant of the name in order to handle non-unicode filenames.
/// Dont't access this field directly, use the [`Node::name()`] method instead!
/// # Warning
///
/// This contains an escaped variant of the name in order to handle non-unicode filenames.
/// Don't access this field directly, use the [`Node::name()`] method instead!
pub name: String,
#[serde(flatten)]
/// Information about node type
Expand All @@ -44,7 +46,9 @@ pub struct Node {
/// Node Metadata
pub meta: Metadata,
#[serde(default, deserialize_with = "deserialize_default_from_null")]
/// Contents of the Node. This should be only set for regular files
/// Contents of the Node
///
/// This should be only set for regular files.
pub content: Option<Vec<Id>>,
#[serde(default, skip_serializing_if = "Option::is_none")]
/// Subtree of the Node. This should be only
Expand All @@ -64,7 +68,9 @@ pub enum NodeType {
Symlink {
/// The target of the symlink
///
/// Warning: This contains the target only if it is a valid unicode target.
/// # Warning
///
/// This contains the target only if it is a valid unicode target.
/// Dont't access this field directly, use the [`NodeType::to_link()`] method instead!
linktarget: String,
#[serde_as(as = "Option<Base64>")]
Expand Down Expand Up @@ -214,25 +220,25 @@ impl Node {
}
}
#[must_use]
/// Is this node a dir?
/// Evaluates if this node is a directory
pub const fn is_dir(&self) -> bool {
matches!(self.node_type, NodeType::Dir)
}

#[must_use]
/// Is this node a symlink?
/// Evaluates if this node is a symlink
pub const fn is_symlink(&self) -> bool {
matches!(self.node_type, NodeType::Symlink { .. })
}

#[must_use]
/// Is this node a regular file?
/// Evaluates if this node is a regular file
pub const fn is_file(&self) -> bool {
matches!(self.node_type, NodeType::File)
}

#[must_use]
/// Is this node a special file?
/// Evaluates if this node is a special file
pub const fn is_special(&self) -> bool {
matches!(
self.node_type,
Expand All @@ -251,8 +257,8 @@ impl Node {
}
}

/// `latest_node` is an ordering function returning the latest node by mtime
pub fn last_modified(n1: &Node, n2: &Node) -> Ordering {
/// `last_modified_node` is an ordering function returning the latest node by mtime
pub fn last_modified_node(n1: &Node, n2: &Node) -> Ordering {
n1.meta.mtime.cmp(&n2.meta.mtime)
}

Expand Down
2 changes: 1 addition & 1 deletion crates/rustic_core/src/commands/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub(crate) fn save_config<P, S>(
#[cfg_attr(feature = "clap", derive(clap::Parser))]
#[derive(Debug, Clone, Copy, Default, Setters)]
#[setters(into)]
/// Options for the `config` command, used to set repository-wide options
/// Options for the `config` command, used to set repository-wide options
pub struct ConfigOptions {
/// Set compression level. Allowed levels are 1 to 22 and -1 to -7, see <https://facebook.github.io/zstd/>.
/// Note that 0 equals to no compression
Expand Down
2 changes: 1 addition & 1 deletion crates/rustic_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub use crate::{
backend::{
ignore::{LocalSource, LocalSourceFilterOptions, LocalSourceSaveOptions},
local::LocalDestination,
node::last_modified,
node::last_modified_node,
ReadSourceEntry,
},
blob::{tree::TreeStreamerOptions as LsOptions, Sum},
Expand Down
9 changes: 2 additions & 7 deletions src/commands/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@ use log::info;

use chrono::Local;

use rustic_core::{
repofile::{Node, SnapshotFile},
SnapshotOptions,
};
use rustic_core::{last_modified_node, repofile::SnapshotFile, SnapshotOptions};

/// `merge` subcommand
#[derive(clap::Parser, Default, Command, Debug)]
Expand Down Expand Up @@ -55,9 +52,7 @@ impl MergeCmd {

let snap = SnapshotFile::from_options(&self.snap_opts)?;

let cmp = |n1: &Node, n2: &Node| n1.meta.mtime.cmp(&n2.meta.mtime);

let snap = repo.merge_snapshots(&snapshots, &cmp, snap)?;
let snap = repo.merge_snapshots(&snapshots, &last_modified_node, snap)?;

if self.json {
let mut stdout = std::io::stdout();
Expand Down

0 comments on commit 34972d3

Please sign in to comment.