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

Refactor decoding errors with more improvements #131

Open
4 tasks
chshersh opened this issue Oct 9, 2022 · 3 comments
Open
4 tasks

Refactor decoding errors with more improvements #131

chshersh opened this issue Oct 9, 2022 · 3 comments
Assignees
Labels
config TOML configuration, config-related CLI options good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/
Milestone

Comments

@chshersh
Copy link
Owner

chshersh commented Oct 9, 2022

Custom decoding errors were implemented in the following PR:

This issue is about some minor improvements to the implementation so they can be done separately:

  • Move Value::String("some value") into const EXPECTED_STRING or STRING_TYPE. It's a hack to reuse toml::Value for types of expected values. We can move them into top-level constants for easier reuse in the future:
  • Return error when asset_name is not a table. Currently we return empty AssetName but we should provide a custom error when it's something like asset_name = "x86_64_unknown_linux_musl". Implement a similar constant to the previous task for string
  • match table.get("asset_name").and_then(|t| t.as_table()) {
    None => AssetName {
    linux: None,
    macos: None,
    windows: None,
    },
  • Return error when str_by_key doesn't see String. The function returns Option<String>. Its type should be changed to Result<Option<String>, DecodeError> and its name should be changed to optional_str_by_key. And this function should throw an error when it sees something besides string.
  • fn str_by_key(table: &Map<String, Value>, key: &str) -> Option<String> {
    table.get(key).and_then(|v| v.as_str()).map(String::from)
    }
  • Throw DecodeError when iterating through the map. We expect all tools to be tables. So we should iterate through the map, filter out expected keys (proxy and store_directory) and throw an error otherwise
@chshersh chshersh added good first issue Good for newcomers config TOML configuration, config-related CLI options hacktoberfest https://hacktoberfest.com/ labels Oct 9, 2022
@chshersh chshersh added this to the v0.3.0: Auto milestone Oct 9, 2022
@chshersh
Copy link
Owner Author

chshersh commented Oct 9, 2022

@DukeManh I created this issue as a follow-up to your improvements in #122. You don't need to implement this issue (unless you want to), someone else could contribute the improvements 🙂 You've created the mechanism but in this issue I propose to use it in even more places.

@ChellappanRajan
Copy link

Hi @chshersh I would like to work on this issue. Can you please assign it on my name?

@chshersh
Copy link
Owner Author

Hi @ChellappanRajan 👋🏻
Sure, go for it 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config TOML configuration, config-related CLI options good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/
Projects
None yet
Development

No branches or pull requests

2 participants