diff --git a/cli/src/argparse/filter.rs b/cli/src/argparse/filter.rs index f381e5862..4645de5da 100644 --- a/cli/src/argparse/filter.rs +++ b/cli/src/argparse/filter.rs @@ -2,6 +2,7 @@ use super::args::{arg_matching, id_list, minus_tag, plus_tag, TaskId}; use super::ArgList; use crate::usage; use nom::{branch::alt, combinator::*, multi::fold_many0, IResult}; +use taskchampion::Status; /// A filter represents a selection of a particular set of tasks. /// @@ -10,40 +11,11 @@ use nom::{branch::alt, combinator::*, multi::fold_many0, IResult}; /// pending tasks, or all tasks. #[derive(Debug, PartialEq, Default, Clone)] pub(crate) struct Filter { - /// The universe of tasks from which this filter can select - pub(crate) universe: Universe, - /// A set of filter conditions, all of which must match a task in order for that task to be /// selected. pub(crate) conditions: Vec, } -/// The universe of tasks over which a filter should be applied. -#[derive(Debug, PartialEq, Clone)] -pub(crate) enum Universe { - /// Only the identified tasks. Note that this may contain duplicates. - IdList(Vec), - /// All tasks in the task database - AllTasks, - /// Only pending tasks (or as an approximation, the working set) - #[allow(dead_code)] // currently only used in tests - PendingTasks, -} - -impl Universe { - /// Testing shorthand to construct a simple universe - #[cfg(test)] - pub(super) fn for_ids(mut ids: Vec) -> Self { - Universe::IdList(ids.drain(..).map(|id| TaskId::WorkingSetId(id)).collect()) - } -} - -impl Default for Universe { - fn default() -> Self { - Self::AllTasks - } -} - /// A condition which tasks must match to be accepted by the filter. #[derive(Debug, PartialEq, Clone)] pub(crate) enum Condition { @@ -52,10 +24,18 @@ pub(crate) enum Condition { /// Task does not have the given tag NoTag(String), + + // TODO: add command-line syntax for this + /// Task has the given status + Status(Status), + + /// Task has one of the given IDs + IdList(Vec), } /// Internal struct representing a parsed filter argument enum FilterArg { + // TODO: get rid of this whole enum IdList(Vec), Condition(Condition), } @@ -67,30 +47,46 @@ impl Filter { Filter { ..Default::default() }, - Self::fold_args, + |acc, arg| acc.with_arg(arg), )(input) } /// fold multiple filter args into a single Filter instance - fn fold_args(mut acc: Filter, mod_arg: FilterArg) -> Filter { + fn with_arg(mut self, mod_arg: FilterArg) -> Filter { match mod_arg { FilterArg::IdList(mut id_list) => { - // If any IDs are specified, then the filter's universe - // is those IDs. If there are already IDs, append to the - // list. - if let Universe::IdList(ref mut existing) = acc.universe { + // If there is already an IdList condition, concatenate this one + // to it. Thus multiple IdList command-line args represent an OR + // operation. This assumes that the filter is still being built + // from command-line arguments and thus has at most one IdList + // condition. + if let Some(Condition::IdList(existing)) = self + .conditions + .iter_mut() + .find(|c| matches!(c, Condition::IdList(_))) + { existing.append(&mut id_list); } else { - acc.universe = Universe::IdList(id_list); + self.conditions.push(Condition::IdList(id_list)); } } FilterArg::Condition(cond) => { - acc.conditions.push(cond); + self.conditions.push(cond); } } - acc + self } + /// combine this filter with another filter in an AND operation + pub(crate) fn intersect(mut self, mut other: Filter) -> Filter { + // simply concatenate the conditions + self.conditions.append(&mut other.conditions); + + self + } + + // parsers + fn id_list(input: ArgList) -> IResult { fn to_filterarg(input: Vec) -> Result { Ok(FilterArg::IdList(input)) @@ -112,6 +108,8 @@ impl Filter { map_res(arg_matching(minus_tag), to_filterarg)(input) } + // usage + pub(super) fn get_usage(u: &mut usage::Usage) { u.filters.push(usage::Filter { syntax: "TASKID[,TASKID,..]", @@ -160,8 +158,7 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![TaskId::WorkingSetId(1)]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(1)])], } ); } @@ -173,12 +170,11 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::WorkingSetId(2), TaskId::WorkingSetId(3), - ]), - ..Default::default() + ])], } ); } @@ -190,13 +186,12 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::WorkingSetId(2), TaskId::WorkingSetId(3), TaskId::WorkingSetId(4), - ]), - ..Default::default() + ])], } ); } @@ -208,11 +203,10 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::PartialUuid(s!("abcd1234")), - ]), - ..Default::default() + ])], } ); } @@ -224,12 +218,66 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![TaskId::WorkingSetId(1),]), conditions: vec![ + Condition::IdList(vec![TaskId::WorkingSetId(1),]), Condition::HasTag("yes".into()), Condition::NoTag("no".into()), ], - ..Default::default() + } + ); + } + + #[test] + fn intersect_idlist_idlist() { + let left = Filter::parse(argv!["1,2", "+yes"]).unwrap().1; + let right = Filter::parse(argv!["2,3", "+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + // from first filter + Condition::IdList(vec![TaskId::WorkingSetId(1), TaskId::WorkingSetId(2),]), + Condition::HasTag("yes".into()), + // from second filter + Condition::IdList(vec![TaskId::WorkingSetId(2), TaskId::WorkingSetId(3)]), + Condition::HasTag("no".into()), + ], + } + ); + } + + #[test] + fn intersect_idlist_alltasks() { + let left = Filter::parse(argv!["1,2", "+yes"]).unwrap().1; + let right = Filter::parse(argv!["+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + // from first filter + Condition::IdList(vec![TaskId::WorkingSetId(1), TaskId::WorkingSetId(2),]), + Condition::HasTag("yes".into()), + // from second filter + Condition::HasTag("no".into()), + ], + } + ); + } + + #[test] + fn intersect_alltasks_alltasks() { + let left = Filter::parse(argv!["+yes"]).unwrap().1; + let right = Filter::parse(argv!["+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + Condition::HasTag("yes".into()), + Condition::HasTag("no".into()), + ], } ); } diff --git a/cli/src/argparse/mod.rs b/cli/src/argparse/mod.rs index 0bf656a8f..ab54326f1 100644 --- a/cli/src/argparse/mod.rs +++ b/cli/src/argparse/mod.rs @@ -19,7 +19,7 @@ mod subcommand; pub(crate) use args::TaskId; pub(crate) use command::Command; -pub(crate) use filter::{Condition, Filter, Universe}; +pub(crate) use filter::{Condition, Filter}; pub(crate) use modification::{DescriptionMod, Modification}; pub(crate) use subcommand::Subcommand; diff --git a/cli/src/argparse/subcommand.rs b/cli/src/argparse/subcommand.rs index 0caf17ebd..1d7ddc2ba 100644 --- a/cli/src/argparse/subcommand.rs +++ b/cli/src/argparse/subcommand.rs @@ -383,7 +383,7 @@ impl Sync { #[cfg(test)] mod test { use super::*; - use crate::argparse::Universe; + use crate::argparse::Condition; const EMPTY: Vec<&str> = vec![]; @@ -459,8 +459,7 @@ mod test { fn test_modify_description_multi() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("foo bar")), @@ -477,8 +476,7 @@ mod test { fn test_append() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Append(s!("foo bar")), @@ -495,8 +493,7 @@ mod test { fn test_prepend() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Prepend(s!("foo bar")), @@ -513,8 +510,7 @@ mod test { fn test_done() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { status: Some(Status::Completed), @@ -531,8 +527,7 @@ mod test { fn test_done_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("now-finished")), @@ -550,8 +545,7 @@ mod test { fn test_start() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(true), @@ -568,8 +562,7 @@ mod test { fn test_start_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(true), @@ -587,8 +580,7 @@ mod test { fn test_stop() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(false), @@ -605,8 +597,7 @@ mod test { fn test_stop_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("mod")), @@ -636,8 +627,10 @@ mod test { fn test_report_filter_before() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "foo".to_owned(), }; @@ -651,8 +644,10 @@ mod test { fn test_report_filter_after() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "foo".to_owned(), }; @@ -666,8 +661,10 @@ mod test { fn test_report_filter_next() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "next".to_owned(), }; @@ -696,8 +693,10 @@ mod test { let subcommand = Subcommand::Info { debug: false, filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, }; assert_eq!( @@ -711,8 +710,7 @@ mod test { let subcommand = Subcommand::Info { debug: true, filter: Filter { - universe: Universe::for_ids(vec![12]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(12)])], }, }; assert_eq!( diff --git a/cli/src/invocation/filter.rs b/cli/src/invocation/filter.rs index f87263270..34fc1b1e1 100644 --- a/cli/src/invocation/filter.rs +++ b/cli/src/invocation/filter.rs @@ -1,10 +1,10 @@ -use crate::argparse::{Condition, Filter, TaskId, Universe}; +use crate::argparse::{Condition, Filter, TaskId}; use failure::Fallible; use std::collections::HashSet; use std::convert::TryInto; -use taskchampion::{Replica, Tag, Task}; +use taskchampion::{Replica, Status, Tag, Task, Uuid}; -fn match_task(filter: &Filter, task: &Task) -> bool { +fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option) -> bool { for cond in &filter.conditions { match cond { Condition::HasTag(ref tag) => { @@ -21,11 +21,85 @@ fn match_task(filter: &Filter, task: &Task) -> bool { return false; } } + Condition::Status(status) => { + if task.get_status() != *status { + return false; + } + } + Condition::IdList(ids) => { + let uuid_str = uuid.to_string(); + let mut found = false; + for id in ids { + if match id { + TaskId::WorkingSetId(i) => Some(*i) == working_set_id, + TaskId::PartialUuid(partial) => uuid_str.starts_with(partial), + TaskId::Uuid(i) => *i == uuid, + } { + found = true; + break; + } + } + if !found { + return false; + } + } } } true } +// the universe of tasks we must consider +enum Universe { + /// Scan all the tasks + AllTasks, + /// Scan the working set (for pending tasks) + WorkingSet, + /// Scan an explicit set of tasks, "Absolute" meaning either full UUID or a working set + /// index + AbsoluteIdList(Vec), +} + +/// Determine the universe for the given filter; avoiding the need to scan all tasks in most cases. +fn universe_for_filter(filter: &Filter) -> Universe { + /// If there is a condition with Status::Pending, return true + fn has_pending_condition(filter: &Filter) -> bool { + filter + .conditions + .iter() + .any(|cond| matches!(cond, Condition::Status(Status::Pending))) + } + + /// If there is a condition with an IdList containing no partial UUIDs, + /// return that. + fn absolute_id_list_condition(filter: &Filter) -> Option> { + filter + .conditions + .iter() + .find(|cond| { + if let Condition::IdList(ids) = cond { + !ids.iter().any(|id| matches!(id, TaskId::PartialUuid(_))) + } else { + false + } + }) + .map(|cond| { + if let Condition::IdList(ids) = cond { + ids.to_vec() + } else { + unreachable!() // any condition found above must be an IdList(_) + } + }) + } + + if let Some(ids) = absolute_id_list_condition(filter) { + Universe::AbsoluteIdList(ids) + } else if has_pending_condition(filter) { + Universe::WorkingSet + } else { + Universe::AllTasks + } +} + /// Return the tasks matching the given filter. This will return each matching /// task once, even if the user specified the same task multiple times on the /// command line. @@ -35,38 +109,14 @@ pub(super) fn filtered_tasks( ) -> Fallible> { let mut res = vec![]; - fn is_partial_uuid(taskid: &TaskId) -> bool { - matches!(taskid, TaskId::PartialUuid(_)) - } + log::debug!("Applying filter {:?}", filter); // We will enumerate the universe of tasks for this filter, checking // each resulting task with match_task - match filter.universe { + match universe_for_filter(filter) { // A list of IDs, but some are partial so we need to iterate over // all tasks and pattern-match their Uuids - Universe::IdList(ref ids) if ids.iter().any(is_partial_uuid) => { - log::debug!("Scanning entire task database due to partial UUIDs in the filter"); - 'task: for (uuid, task) in replica.all_tasks()?.drain() { - for id in ids { - let in_universe = match id { - TaskId::WorkingSetId(id) => { - // NOTE: (#108) this results in many reads of the working set; it - // may be better to cache this information here or in the Replica. - replica.get_working_set_index(&uuid)? == Some(*id) - } - TaskId::PartialUuid(prefix) => uuid.to_string().starts_with(prefix), - TaskId::Uuid(id) => id == &uuid, - }; - if in_universe && match_task(filter, &task) { - res.push(task); - continue 'task; - } - } - } - } - - // A list of full IDs, which we can fetch directly - Universe::IdList(ref ids) => { + Universe::AbsoluteIdList(ref ids) => { log::debug!("Scanning only the tasks specified in the filter"); // this is the only case where we might accidentally return the same task // several times, so we must track the seen tasks. @@ -74,7 +124,7 @@ pub(super) fn filtered_tasks( for id in ids { let task = match id { TaskId::WorkingSetId(id) => replica.get_working_set_task(*id)?, - TaskId::PartialUuid(_) => unreachable!(), // handled above + TaskId::PartialUuid(_) => unreachable!(), // not present in absolute id list TaskId::Uuid(id) => replica.get_task(id)?, }; @@ -86,7 +136,9 @@ pub(super) fn filtered_tasks( } seen.insert(uuid); - if match_task(filter, &task) { + let working_set_id = replica.get_working_set_index(&uuid)?; + + if match_task(filter, &task, uuid, working_set_id) { res.push(task); } } @@ -96,19 +148,20 @@ pub(super) fn filtered_tasks( // All tasks -- iterate over the full set Universe::AllTasks => { log::debug!("Scanning all tasks in the task database"); - for (_, task) in replica.all_tasks()?.drain() { - if match_task(filter, &task) { + for (uuid, task) in replica.all_tasks()?.drain() { + // Yikes, slow! https://github.com/djmitche/taskchampion/issues/108 + let working_set_id = replica.get_working_set_index(&uuid)?; + if match_task(filter, &task, uuid, working_set_id) { res.push(task); } } } - - // Pending tasks -- just scan the working set - Universe::PendingTasks => { + Universe::WorkingSet => { log::debug!("Scanning only the working set (pending tasks)"); - for task in replica.working_set()?.drain(..) { + for (i, task) in replica.working_set()?.drain(..).enumerate() { if let Some(task) = task { - if match_task(filter, &task) { + let uuid = *task.get_uuid(); + if match_task(filter, &task, uuid, Some(i)) { res.push(task); } } @@ -136,12 +189,11 @@ mod test { let t1uuid = *t1.get_uuid(); let filter = Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::Uuid(t1uuid), // A TaskId::WorkingSetId(1), // A (again, dups filtered) TaskId::Uuid(*t2.get_uuid()), // B - ]), - ..Default::default() + ])], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() @@ -165,12 +217,11 @@ mod test { let t2partial = t2uuid[..13].to_owned(); let filter = Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::Uuid(t1uuid), // A TaskId::WorkingSetId(1), // A (again, dups filtered) TaskId::PartialUuid(t2partial), // B - ]), - ..Default::default() + ])], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() @@ -189,10 +240,7 @@ mod test { replica.new_task(Status::Deleted, s!("C")).unwrap(); replica.gc().unwrap(); - let filter = Filter { - universe: Universe::AllTasks, - ..Default::default() - }; + let filter = Filter { conditions: vec![] }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() .map(|t| t.get_description().to_owned()) @@ -224,9 +272,7 @@ mod test { // look for just "yes" (A and B) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::HasTag(s!("yes"))], - ..Default::default() }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -236,9 +282,7 @@ mod test { // look for tags without "no" (A, D) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::NoTag(s!("no"))], - ..Default::default() }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -248,9 +292,7 @@ mod test { // look for tags with "yes" and "no" (B) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::HasTag(s!("yes")), Condition::HasTag(s!("no"))], - ..Default::default() }; let filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -270,8 +312,7 @@ mod test { replica.gc().unwrap(); let filter = Filter { - universe: Universe::PendingTasks, - ..Default::default() + conditions: vec![Condition::Status(Status::Pending)], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index 0ed793f7d..82cc67bba 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -1,11 +1,11 @@ -use crate::argparse::Filter; +use crate::argparse::{Condition, Filter}; use crate::invocation::filtered_tasks; use crate::report::{Column, Property, Report, Sort, SortBy}; use crate::table; use failure::{bail, Fallible}; use prettytable::{Row, Table}; use std::cmp::Ordering; -use taskchampion::{Replica, Task, Uuid}; +use taskchampion::{Replica, Status, Task, Uuid}; use termcolor::WriteColor; // pending #123, this is a non-fallible way of looking up a task's working set index @@ -125,24 +125,26 @@ fn get_report(report_name: String, filter: Filter) -> Fallible { ascending: false, sort_by: SortBy::Uuid, }]; - use crate::argparse::Universe; - Ok(match report_name.as_ref() { + let mut report = match report_name.as_ref() { "list" => Report { columns, sort, - filter, + filter: Default::default(), }, "next" => Report { columns, sort, - // TODO: merge invocation filter and report filter filter: Filter { - universe: Universe::PendingTasks, - ..Default::default() + conditions: vec![Condition::Status(Status::Pending)], }, }, _ => bail!("Unknown report {:?}", report_name), - }) + }; + + // intersect the report's filter with the user-supplied filter + report.filter = report.filter.intersect(filter); + + Ok(report) } pub(super) fn display_report(