Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add human-readable format (serialization only) #42

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

max-huster
Copy link

Hey, i needed this for my project. Maybe it could be helpful to others as well.

@max-huster
Copy link
Author

I realized that there are some limitations to the current implementaion. A Seperate sub-command to only convert the format (toml, json, yaml) to text would be better... I will modify the code as well as the README very soon.

@sstadick
Copy link
Owner

sstadick commented Jan 7, 2025

That's a great idea! Thanks for the PR, I'd very much like to get this in.

@max-huster
Copy link
Author

Hi, i modifed the code slightly now it works as intented. I also formatted the files to the linter won't cry at me :)

Copy link
Owner

@sstadick sstadick left a comment

Choose a reason for hiding this comment

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

Overall this looks great! I just want to nail down the naming of the new flag / make it clear how it's working via the name since it kind of short circuits the checking logic.

Comment on lines +75 to +76
#[structopt(long, short)]
human_readable_artifact: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

question: would you be opposed to adding HumanReadable as a format::Format?

And then we could avoid a special-case flag and handling for just outputing a text file.

Alternatively, I think adding a subcommand for this, or calling this flag convert_to_human_readable_only or something like that to make it clear what this is doing.

mut output: Box<dyn Write + Send>,
) -> Result<(), std::io::Error> {
let content = format!("{self}");
output.write_all(content.as_bytes())?;
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: would `write!(&mut output, "{}", self)?; work here? That may avoid creating the intermediate string.

@@ -122,6 +135,10 @@ impl Bundle {
}
}

pub fn get_third_party_libraries(&self) -> Vec<FinalizedLicense> {
Copy link
Owner

Choose a reason for hiding this comment

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

question: is this used somewhere else? If not can it be removed?

Currently the supported formats are `json`, `yaml`, and `toml`. A more human readable format that is closer to a classical THIRDPARTY file and already has `serde` support is being actively sought. Please create an issue or PR if you have an idea for this.
Currently the supported formats are `json`, `yaml`, and `toml`.

You can also export those previously exported (and maybe edited) files to a human-readable format with `--human-readable-artifact`
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
You can also export those previously exported (and maybe edited) files to a human-readable format with `--human-readable-artifact`
You can also convert those previously exported (and maybe edited) files to a human-readable format with `--human-readable-artifact`

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.

2 participants