Skip to content

Use default values for properties missing from --settings file#125

Merged
sameer merged 3 commits into
sameer:mainfrom
AmadeusW:useDefaultsForMissingSettings
Jul 15, 2026
Merged

Use default values for properties missing from --settings file#125
sameer merged 3 commits into
sameer:mainfrom
AmadeusW:useDefaultsForMissingSettings

Conversation

@AmadeusW

@AmadeusW AmadeusW commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Use default values for properties missing from --settings file.

This allows for a minimal settings file, such as

{
  "conversion": {
    "feedrate": 4500
  },
  "postprocess": {
    "newline_before_comment": true
  }
}

Comment thread cli/src/main.rs Outdated
Comment thread g_code/src/config/mod.rs

/// A cross-platform type used to store all configuration types.
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "serde", serde(default))]

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.

Without this default, we'd need to create settings with empty objects, e.g. { "conversion": {} }

Comment thread star/src/lower/mod.rs
/// Dots per inch for pixels, picas, points, etc.
pub dpi: f64,
/// Set the origin point in millimeters for this conversion
#[cfg_attr(feature = "serde", serde(default = "zero_origin"))]

@AmadeusW AmadeusW Jul 15, 2026

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.

This is OK to remove because impl Default for ConversionConfig { on line 56 provides the zero_origin default. At the same time, this might be less obvious, so let me know what you think

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Lgtm, thanks!

@sameer sameer merged commit 1a36229 into sameer:main Jul 15, 2026
3 of 4 checks passed
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