Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

Commit

Permalink
fix output, fix previous ignored clippy warnings
Browse files Browse the repository at this point in the history
* as of the error reworking commit, tasks sometimes didn't erase
  lines when it should have. most specifically at the beginning
  of tasks being executed, and all tasks completing. this is now
  fixed, as the task_inidcator ignores logs that happened before
  it started, and properly counts how many lines it added, that it
  needs to erase.

* we've reworked all of the codebase, so almost all clippy warnings
  that were previously ignored, are no longer ignored, and code
  has been rewritten so the lint step passes.

  * There are some "clippy::too_many_arguments" being ignored,
    I intend to fix these, but these require more of a deft hand,
    and so I don't want to rush it. reworking may also not be needed
    once I try it. (This is currently the most ignored clippy warning
    according too: rust-lang/rust-clippy#5418)
    and reasoning I agree with is there.
  * There is one: `#[allow(unused)]` for pipeline descriptions. This
    is true, it is unused right now, but I want to keep it in the
    schema because I do want to render the pipeline in list one day.
    I just haven't found quite a way to do it yet, but i added it in
    because I know I want to do it (and I don't want to remove it).

* this has also resulted in a giant split up of docker executor,
  so it is no longer all in one gigantic file. This really _really_
  needed to be done, so I'm glad I finally did it.
  • Loading branch information
Mythra committed Jul 10, 2020
1 parent 5e7809d commit c9144f1
Show file tree
Hide file tree
Showing 32 changed files with 2,550 additions and 2,274 deletions.
6 changes: 3 additions & 3 deletions src/commands/clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! circumstances where we like powered off while running. However, it should
//! always be safe to run.
use crate::executors::{docker::DockerExecutor, host::HostExecutor};
use crate::executors::{docker, host};

use color_eyre::Result;
use tracing::info;
Expand All @@ -19,8 +19,8 @@ pub async fn handle_clean_command() -> Result<()> {

info!("Cleaning resources ...");

HostExecutor::clean().await;
DockerExecutor::clean().await?;
host::Executor::clean().await;
docker::Executor::clean().await?;

info!("Cleaned.");
Ok(())
Expand Down
3 changes: 1 addition & 2 deletions src/commands/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ fn report_potential_internal_task_names<T>(
/// - Error creating an executor/choosing an executor for tasks.
/// - Error writing the helper scripts.
/// - Error running the task.
#[allow(clippy::cognitive_complexity)]
pub async fn handle_exec_command(
config: &TopLevelConf,
fetcher: &FetcherRepository,
Expand Down Expand Up @@ -204,7 +203,7 @@ pub async fn handle_exec_command(
Ok(exit_code) => {
if exit_code == 0 {
// Don't cause an error for cleaning if the task succeeded, the user can always clean manually.
let _ = crate::executors::docker::DockerExecutor::clean().await;
let _ = crate::executors::docker::Executor::clean().await;
Ok(())
} else {
Err(eyre!(
Expand Down
143 changes: 85 additions & 58 deletions src/commands/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use crate::{
config::types::{OneofOption, TaskConf, TaskType, TopLevelConf},
fetch::FetcherRepository,
strsim::calculate_did_you_mean_possibilities,
tasks::TaskGraph,
terminal::TERM,
};
Expand Down Expand Up @@ -95,6 +96,52 @@ fn turn_oneof_into_listable(options: Option<&Vec<OneofOption>>) -> Vec<(String,
results
}

/// Check if an argument is a selectable top level argument aka:
///
/// 1. Is a task.
/// 2. Is a `Oneof` type.
/// 3. Is not marked as internal.
fn is_selectable_top_level_arg<'a, 'b>(
arg: &'b str,
tasks: &'a HashMap<String, TaskConf>,
) -> Option<&'a TaskConf> {
if !tasks.contains_key(arg) {
error!(
"Argument #1 ({}) is not a task that exists. Listing all possible tasks.",
arg,
);
return None;
}

let selected_task = &tasks[arg];
if selected_task.get_type() != &TaskType::Oneof {
error!(
"Argument #1 ({}) is not a task that can be listed. Listing all possible tasks.",
arg,
);
return None;
}

if selected_task.is_internal() {
error!(
"Argument #1 ({}) is an internal task, and cannot be listed. Listing all the possible tasks.",
arg,
);
return None;
}

Some(selected_task)
}

/// Check if a task has options that can be selected.
fn task_has_options(task: &TaskConf) -> bool {
if let Some(options) = task.get_options() {
!options.is_empty()
} else {
false
}
}

/// Handle a raw list configuration.
///
/// `config`: The configuration object.
Expand Down Expand Up @@ -134,61 +181,33 @@ fn handle_raw_list(config: &TopLevelConf, tasks: &HashMap<String, TaskConf>) {
);
}

/// Handle the actual `list command`.
///
/// `config` - the top level configuration object.
/// `fetcher` - the thing that goes and fetches for us.
/// `args` - the arguments for this list command.
///
/// # Errors
///
/// - When constructing the task graph.
#[allow(clippy::cognitive_complexity, clippy::too_many_lines)]
pub async fn handle_list_command(
config: &TopLevelConf,
fetcher: &FetcherRepository,
args: &[String],
) -> Result<()> {
let span = tracing::info_span!("list");
let _guard = span.enter();

// The list command is the main command you get when running the binary.
// It is not really ideal for CI environments, and we really only ever
// expect humans to run it. This is why we specifically colour it, and
// try to always output _something_.
let tasks = TaskGraph::new(config, fetcher)
.await?
.consume_and_get_tasks();

fn handle_listing_arg<'a, 'b>(
tasks: &'a HashMap<String, TaskConf>,
args: &'b [String],
) -> Option<&'a TaskConf> {
let mut last_selected_task: Option<&TaskConf> = None;

for (arg_idx, arg) in args.iter().enumerate() {
if let Some(prior_task) = last_selected_task {
let empty_vec = Vec::with_capacity(0);
if prior_task
.get_options()
.unwrap_or_else(|| &empty_vec)
.is_empty()
{
if !task_has_options(prior_task) {
error!(
"Argument #{} ({}) could not be found since the previous argument: #{} ({}) has no options. Performing top level list.",
arg_idx + 1,
arg,
arg_idx,
prior_task.get_name(),
);
handle_raw_list(config, &tasks);

return Ok(());
return None;
}

let options = prior_task.get_options().unwrap();
let potential_current_option = options.iter().find(|item| item.get_name() == arg);

// Don't check for internal task here since a oneof could be built off
// of internal options, and we want those to be selectable.

if potential_current_option.is_none() {
let did_you_mean_options = crate::strsim::calculate_did_you_mean_possibilities(
let did_you_mean_options = calculate_did_you_mean_possibilities(
arg,
&options
.iter()
Expand Down Expand Up @@ -239,31 +258,39 @@ pub async fn handle_list_command(

last_selected_task = Some(selected_task);
} else {
if !tasks.contains_key(arg) {
error!(
"Argument #1 ({}) is not a task that exists. Listing all possible tasks.",
arg
);
handle_raw_list(config, &tasks);
return Ok(());
}
let arg = is_selectable_top_level_arg(arg, tasks)?;
last_selected_task = Some(arg);
}
}

let selected_task = &tasks[arg];
if selected_task.get_type() != &TaskType::Oneof {
error!("Argument #1 ({}) is not a task that can be listed. Listing all possible tasks.", arg);
handle_raw_list(config, &tasks);
return Ok(());
}
last_selected_task
}

if selected_task.is_internal() {
error!("Argument #1 ({}) is an internal task, and cannot be listed. Listing all the possible tasks.", arg);
handle_raw_list(config, &tasks);
return Ok(());
}
/// Handle the actual `list command`.
///
/// `config` - the top level configuration object.
/// `fetcher` - the thing that goes and fetches for us.
/// `args` - the arguments for this list command.
///
/// # Errors
///
/// - When constructing the task graph.
pub async fn handle_list_command(
config: &TopLevelConf,
fetcher: &FetcherRepository,
args: &[String],
) -> Result<()> {
let span = tracing::info_span!("list");
let _guard = span.enter();

last_selected_task = Some(selected_task);
}
}
// The list command is the main command you get when running the binary.
// It is not really ideal for CI environments, and we really only ever
// expect humans to run it. This is why we specifically colour it, and
// try to always output _something_.
let tasks = TaskGraph::new(config, fetcher)
.await?
.consume_and_get_tasks();
let last_selected_task = handle_listing_arg(&tasks, args);

if last_selected_task.is_none() {
handle_raw_list(config, &tasks);
Expand Down
8 changes: 4 additions & 4 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! The overarching module for all particular commands implemented by Dev-Loop.
pub mod clean;
pub mod exec;
pub mod list;
pub mod run;
pub(crate) mod clean;
pub(crate) mod exec;
pub(crate) mod list;
pub(crate) mod run;
3 changes: 1 addition & 2 deletions src/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use std::path::PathBuf;
/// - Error creating an executor/choosing an executor for tasks.
/// - Error writing the helper scripts.
/// - Error running the task.
#[allow(clippy::cognitive_complexity)]
pub async fn handle_run_command(
config: &TopLevelConf,
fetcher: &FetcherRepository,
Expand Down Expand Up @@ -112,7 +111,7 @@ pub async fn handle_run_command(
Ok(exit_code) => {
if exit_code == 0 {
// Don't cause an error for cleaning if the task succeeded, the user can always clean manually.
let _ = crate::executors::docker::DockerExecutor::clean().await;
let _ = crate::executors::docker::Executor::clean().await;
Ok(())
} else {
Err(eyre!(
Expand Down
9 changes: 4 additions & 5 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//!
//! Those validations happen at different stages within the program.
use crate::yaml_err::contextualize_yaml_err;
use crate::yaml_err::contextualize;
use color_eyre::{eyre::WrapErr, Result, Section};
use std::{
fs::{canonicalize, File},
Expand All @@ -13,13 +13,12 @@ use std::{
};
use tracing::{error, trace};

pub mod types;
pub(crate) mod types;

/// Get the root of the project repository.
///
/// This discovers the project directory automatically by looking at
/// `std::env::current_dir()`, and walking the path up.
#[allow(clippy::cognitive_complexity)]
#[must_use]
pub fn get_project_root() -> Option<PathBuf> {
// Get the current directory (this is where we start looking...)
Expand Down Expand Up @@ -79,7 +78,7 @@ fn find_and_open_project_config() -> Option<(File, PathBuf)> {
/// # Errors
///
/// - When there is error doing a file read on a found configuration file.
pub fn get_top_level_config() -> Result<Option<types::TopLevelConf>> {
pub fn get_top_level() -> Result<Option<types::TopLevelConf>> {
let config_fh_opt = find_and_open_project_config();
if config_fh_opt.is_none() {
error!("Could not find project configuration [.dl/config.yml] looking in current directory, and parent directories.");
Expand All @@ -92,7 +91,7 @@ pub fn get_top_level_config() -> Result<Option<types::TopLevelConf>> {
config_fh.read_to_string(&mut contents)?;

Ok(Some(
contextualize_yaml_err(
contextualize(
serde_yaml::from_str::<types::TopLevelConf>(&contents),
".dl/config.yml",
&contents,
Expand Down
15 changes: 3 additions & 12 deletions src/config/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub struct ExecutorConf {

impl ProvideConf {
/// Create a new implementation of `ProvideConf`
#[cfg(test)]
#[must_use]
pub fn new(name: String, version: Option<String>) -> Self {
Self { name, version }
Expand Down Expand Up @@ -274,6 +275,7 @@ pub struct NeedsRequirement {

impl NeedsRequirement {
/// Create a new `NeedsRequirements`.
#[cfg(test)]
#[must_use]
pub fn new(name: String, version_matcher: Option<String>) -> Self {
Self {
Expand Down Expand Up @@ -332,6 +334,7 @@ impl PipelineStep {
}

/// Get the description of this `PipelineStep`
#[allow(unused)]
#[must_use]
pub fn get_description(&self) -> Option<&str> {
if let Some(desc) = &self.description {
Expand Down Expand Up @@ -564,12 +567,6 @@ pub struct TaskConfFile {
}

impl TaskConfFile {
/// Get the list of tasks from this configuration file.
#[must_use]
pub fn get_tasks(&self) -> &[TaskConf] {
&self.tasks
}

/// Set the task location for all the configuration items
/// in this file.
pub fn set_task_location(&mut self, loc: &str) {
Expand All @@ -593,12 +590,6 @@ pub struct ExecutorConfFile {
}

impl ExecutorConfFile {
/// Get the list of executors that this file added.
#[must_use]
pub fn get_executors(&self) -> &[ExecutorConf] {
&self.executors
}

/// Consume the representation of this `ExecutorConfFile`, and receive the executors.
#[must_use]
pub fn consume_and_get_executors(self) -> Vec<ExecutorConf> {
Expand Down
Loading

0 comments on commit c9144f1

Please sign in to comment.