-
Notifications
You must be signed in to change notification settings - Fork 12
feat!: enforce #[non_exhaustive] policy via clippy lints #299
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize}; | |
| /// Path format (POSIX, Windows, or URI). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "UPPERCASE")] | ||
| #[non_exhaustive] | ||
| pub enum PathFormat { | ||
| #[serde(alias = "posix", alias = "Posix")] | ||
| Posix, | ||
|
|
@@ -36,13 +37,40 @@ impl PathFormat { | |
| /// <https://github.com/OpenJobDescription/openjd-specifications/wiki/How-Jobs-Are-Run#path-mapping> | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| #[non_exhaustive] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have OpenJobDescription/openjd-specifications#47 suggesting to add the destination path format here, which seems like a good idea to me. But, that would be a breaking change so I don't think non_exhaustive is right for this. |
||
| pub struct PathMappingRule { | ||
| pub source_path_format: PathFormat, | ||
| pub source_path: String, | ||
| pub destination_path: String, | ||
| } | ||
|
|
||
| impl PathMappingRule { | ||
| /// Construct a path mapping rule. | ||
| /// | ||
| /// Prefer this over a struct literal: `PathMappingRule` is | ||
| /// `#[non_exhaustive]`, so literal construction is not available to | ||
| /// other crates. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use openjd_expr::{PathFormat, PathMappingRule}; | ||
| /// | ||
| /// let rule = PathMappingRule::new(PathFormat::Posix, "/mnt/shared", "Z:\\shared"); | ||
| /// assert_eq!(rule.source_path, "/mnt/shared"); | ||
| /// ``` | ||
| pub fn new( | ||
| source_path_format: PathFormat, | ||
| source_path: impl Into<String>, | ||
| destination_path: impl Into<String>, | ||
| ) -> Self { | ||
| Self { | ||
| source_path_format, | ||
| source_path: source_path.into(), | ||
| destination_path: destination_path.into(), | ||
| } | ||
| } | ||
|
|
||
| /// Apply this rule using host-native output separators. | ||
| /// Equivalent to Python's behavior (uses `os.name` to pick separator). | ||
| pub fn apply(&self, path: &str) -> Option<String> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,7 @@ impl ExprExtension { | |
| /// the previous split between `FunctionLibrary::with_host_context` and | ||
| /// `FunctionLibrary::with_unresolved_host_context`. | ||
| #[derive(Debug, Clone, Default)] | ||
| #[non_exhaustive] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why shouldn't this one be exhaustive? |
||
| pub enum HostContext { | ||
| /// No host-context functions are registered. Default. | ||
| #[default] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this exhaustive because supporting a new path format seems worth doing a breaking change for.