Skip to content

feat(cli): Add generate subcommand#1168

Merged
Jeffail merged 2 commits intomasterfrom
add-generate-subcmd
Nov 14, 2019
Merged

feat(cli): Add generate subcommand#1168
Jeffail merged 2 commits intomasterfrom
add-generate-subcmd

Conversation

@Jeffail
Copy link
Copy Markdown
Contributor

@Jeffail Jeffail commented Nov 11, 2019

For most components this will simply generate an empty table with
boilerplate fields (type, inputs, etc). Eventually, however,
components will have curated examples written for them which are baked
into the Vector binary and can be used to generate more useful files.

Signed-off-by: Ashley Jeffs ash@jeffail.uk

For most components this will simply generate an empty table with
boilerplate fields (`type`, `inputs`, etc). Eventually, however,
components will have curated examples written for them which are baked
into the Vector binary and can be used to generate more useful files.

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@Jeffail
Copy link
Copy Markdown
Contributor Author

Jeffail commented Nov 11, 2019

Closes #1058

@binarylogic
Copy link
Copy Markdown
Contributor

Docs look great 👍

Copy link
Copy Markdown
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments on ways the implementation could maybe be more idiomatic, but I don't think that needs to block merging. Looks good!

Comment on lines +41 to +63
pub struct SinkOuter {
pub healthcheck: bool,
pub inputs: Vec<String>,
#[serde(flatten)]
pub inner: Value,
pub buffer: crate::buffers::BufferConfig,
}

#[derive(Serialize)]
pub struct TransformOuter {
pub inputs: Vec<String>,
#[serde(flatten)]
pub inner: Value,
}

#[derive(Serialize, Default)]
pub struct Config {
#[serde(flatten)]
pub global: GlobalOptions,
pub sources: IndexMap<String, Value>,
pub transforms: IndexMap<String, TransformOuter>,
pub sinks: IndexMap<String, SinkOuter>,
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little funky to have "duplicates" of these structs just for this purpose, but it's probably simpler than trying to unify.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it felt a bit off, but it does give us tighter control over what gets generated. I've just updated the sections to be optionals to avoid generating empty sections.

src/generate.rs Outdated
Comment on lines +66 to +76
let components: Vec<Vec<String>> = opts
.expression
.split('|')
.map(|s| {
s.to_owned()
.split(',')
.map(|s| s.trim().to_owned())
.filter(|s| s.len() > 0)
.collect()
})
.collect();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like we actually need owned strings here, so you can probably drop all of the to_owned calls. The type annotation can also just be let components: Vec<Vec<_>>.

src/generate.rs Outdated
Comment on lines +85 to +90
components
.get(0)
.unwrap_or(&Vec::new())
.iter()
.for_each(|c| {
i += 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little tricky to follow. It might be a little more idiomatic to do something like

if let Some(source_types) = components.get(0) {
    for (i, source_type) in source_types.iter().enumerate() {
        let name = ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks much nicer now.

src/generate.rs Outdated
Comment on lines +93 to +108
config.sources.insert(name, {
let mut d = SourceDescription::example(c)
.map_err(|e| {
match e {
ExampleError::MissingExample => {}
_ => errs.push(e.clone()),
}
e
})
.unwrap_or(Value::Table(BTreeMap::new()));
d.as_table_mut().map(|s| {
s.insert("type".to_owned(), c.to_owned().into());
s
});
d
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of nesting going on here too. My shot at a more idiomatic version:

let example = match SourceDescription::example(c) {
    Ok(example) => example,
    Err(err) => {
        if err != ExampleError::MissingExample {
            errs.push(e);
        }
        Value::Table(BTreeMap::new())
    }
}
example.as_table_mut().expect("examples are always tables").insert("type".into(), c.into()); 
config.sources.insert(name, example);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, thanks!

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@Jeffail Jeffail merged commit e503057 into master Nov 14, 2019
@Jeffail Jeffail deleted the add-generate-subcmd branch November 14, 2019 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants