Make EnvStackSetResult use Rust naming conventions

src/builtins/abbr.rs

@@ -418,12 +418,12 @@ fn abbr_erase(opts: &Options, parser: &Parser) -> Option<c_int> {
         return STATUS_CMD_ERROR;
     }
 
-    // Erase each. If any is not found, return ENV_NOT_FOUND which is historical.
+    // Erase each. If any is not found, return NotFound which is historical.
     abbrs::with_abbrs_mut(|abbrs| -> Option<c_int> {
         let mut result = STATUS_CMD_OK;
         for arg in &opts.args {
             if !abbrs.erase(arg) {
-                result = Some(EnvStackSetResult::ENV_NOT_FOUND.into());
+                result = Some(EnvStackSetResult::NotFound.into());
             }
             // Erase the old uvar - this makes `abbr -e` work.
             let esc_src = escape(arg);
@@ -431,7 +431,7 @@ fn abbr_erase(opts: &Options, parser: &Parser) -> Option<c_int> {
                 let var_name = WString::from_str("_fish_abbr_") + esc_src.as_utfstr();
                 let ret = parser.vars().remove(&var_name, EnvMode::UNIVERSAL);
 
-                if ret == EnvStackSetResult::ENV_OK {
+                if ret == EnvStackSetResult::Ok {
                     result = STATUS_CMD_OK
                 };
             }

src/builtins/set.rs

@@ -310,29 +310,29 @@ fn warn_if_uvar_shadows_global(
 
 fn handle_env_return(retval: EnvStackSetResult, cmd: &wstr, key: &wstr, streams: &mut IoStreams) {
     match retval {
-        EnvStackSetResult::ENV_OK => (),
-        EnvStackSetResult::ENV_PERM => {
+        EnvStackSetResult::Ok => (),
+        EnvStackSetResult::Perm => {
             streams.err.append(wgettext_fmt!(
                 "%ls: Tried to change the read-only variable '%ls'\n",
                 cmd,
                 key
             ));
         }
-        EnvStackSetResult::ENV_SCOPE => {
+        EnvStackSetResult::Scope => {
             streams.err.append(wgettext_fmt!(
                 "%ls: Tried to modify the special variable '%ls' with the wrong scope\n",
                 cmd,
                 key
             ));
         }
-        EnvStackSetResult::ENV_INVALID => {
+        EnvStackSetResult::Invalid => {
             streams.err.append(wgettext_fmt!(
                 "%ls: Tried to modify the special variable '%ls' to an invalid value\n",
                 cmd,
                 key
             ));
         }
-        EnvStackSetResult::ENV_NOT_FOUND => {
+        EnvStackSetResult::NotFound => {
             streams.err.append(wgettext_fmt!(
                 "%ls: The variable '%ls' does not exist\n",
                 cmd,
@@ -780,13 +780,13 @@ fn erase(
             if split.indexes.is_empty() {
                 // unset the var
                 retval = parser.vars().remove(split.varname, scope);
-                // When a non-existent-variable is unset, return ENV_NOT_FOUND as $status
+                // When a non-existent-variable is unset, return NotFound as $status
                 // but do not emit any errors at the console as a compromise between user
                 // friendliness and correctness.
-                if retval != EnvStackSetResult::ENV_NOT_FOUND {
+                if retval != EnvStackSetResult::NotFound {
                     handle_env_return(retval, cmd, split.varname, streams);
                 }
-                if retval == EnvStackSetResult::ENV_OK && !opts.no_event {
+                if retval == EnvStackSetResult::Ok && !opts.no_event {
                     event::fire(parser, Event::variable_erase(split.varname.to_owned()));
                 }
             } else {
@@ -808,7 +808,7 @@ fn erase(
 
             // Set $status to the last error value.
             // This is cheesy, but I don't expect this to be checked often.
-            if retval != EnvStackSetResult::ENV_OK {
+            if retval != EnvStackSetResult::Ok {
                 ret = env_result_to_status(retval);
             }
         }
@@ -818,11 +818,11 @@ fn erase(
 
 fn env_result_to_status(retval: EnvStackSetResult) -> Option<c_int> {
     Some(match retval {
-        EnvStackSetResult::ENV_OK => 0,
-        EnvStackSetResult::ENV_PERM => 1,
-        EnvStackSetResult::ENV_SCOPE => 2,
-        EnvStackSetResult::ENV_INVALID => 3,
-        EnvStackSetResult::ENV_NOT_FOUND => 4,
+        EnvStackSetResult::Ok => 0,
+        EnvStackSetResult::Perm => 1,
+        EnvStackSetResult::Scope => 2,
+        EnvStackSetResult::Invalid => 3,
+        EnvStackSetResult::NotFound => 4,
     })
 }
 
@@ -981,7 +981,7 @@ fn set_internal(
     let retval =
         env_set_reporting_errors(cmd, opts, split.varname, scope, new_values, streams, parser);
 
-    if retval == EnvStackSetResult::ENV_OK {
+    if retval == EnvStackSetResult::Ok {
         warn_if_uvar_shadows_global(cmd, opts, split.varname, streams, parser);
     }
     env_result_to_status(retval)

src/env/environment.rs

@@ -42,27 +42,27 @@ static UVARS_LOCALLY_MODIFIED: RelaxedAtomicBool = RelaxedAtomicBool::new(false)
 /// Return values for `EnvStack::set()`.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub enum EnvStackSetResult {
-    ENV_OK,
-    ENV_PERM,
-    ENV_SCOPE,
-    ENV_INVALID,
-    ENV_NOT_FOUND,
+    Ok,       // The variable was set successfully.
+    Perm,     // The variable is read-only.
+    Scope,    // Variable cannot be set in the given scope.
+    Invalid,  // The variable's value is invalid (e.g. umask).
+    NotFound, // The variable was not found (only possible when removing a variable).
 }
 
 impl Default for EnvStackSetResult {
     fn default() -> Self {
-        EnvStackSetResult::ENV_OK
+        EnvStackSetResult::Ok
     }
 }
 
 impl From<EnvStackSetResult> for c_int {
     fn from(r: EnvStackSetResult) -> Self {
         match r {
-            EnvStackSetResult::ENV_OK => 0,
-            EnvStackSetResult::ENV_PERM => 1,
-            EnvStackSetResult::ENV_SCOPE => 2,
-            EnvStackSetResult::ENV_INVALID => 3,
-            EnvStackSetResult::ENV_NOT_FOUND => 4,
+            EnvStackSetResult::Ok => 0,
+            EnvStackSetResult::Perm => 1,
+            EnvStackSetResult::Scope => 2,
+            EnvStackSetResult::Invalid => 3,
+            EnvStackSetResult::NotFound => 4,
         }
     }
 }
@@ -228,7 +228,7 @@ impl EnvStack {
         }
 
         let ret: ModResult = self.lock().set(key, mode, vals);
-        if ret.status == EnvStackSetResult::ENV_OK {
+        if ret.status == EnvStackSetResult::Ok {
             // If we modified the global state, or we are principal, then dispatch changes.
             // Important to not hold the lock here.
             if ret.global_modified || self.is_principal() {
@@ -277,7 +277,7 @@ impl EnvStack {
     pub fn remove(&self, key: &wstr, mode: EnvMode) -> EnvStackSetResult {
         let ret = self.lock().remove(key, mode);
         #[allow(clippy::collapsible_if)]
-        if ret.status == EnvStackSetResult::ENV_OK {
+        if ret.status == EnvStackSetResult::Ok {
             if ret.global_modified || self.is_principal() {
                 // Important to not hold the lock here.
                 env_dispatch_var_change(key, self);

src/env/environment_impl.rs

@@ -65,10 +65,10 @@ fn next_export_generation() -> ExportGeneration {
 
 fn set_umask(list_val: &Vec<WString>) -> EnvStackSetResult {
     if list_val.len() != 1 || list_val[0].is_empty() {
-        return EnvStackSetResult::ENV_INVALID;
+        return EnvStackSetResult::Invalid;
     }
     let Ok(mask) = fish_wcstol_radix(&list_val[0], 8) else {
-        return EnvStackSetResult::ENV_INVALID;
+        return EnvStackSetResult::Invalid;
     };
 
     #[allow(
@@ -77,12 +77,12 @@ fn set_umask(list_val: &Vec<WString>) -> EnvStackSetResult {
         clippy::absurd_extreme_comparisons
     )]
     if mask > 0o777 || mask < 0 {
-        return EnvStackSetResult::ENV_INVALID;
+        return EnvStackSetResult::Invalid;
     }
     // Do not actually create a umask variable. On env_stack_t::get() it will be calculated.
     // SAFETY: umask cannot fail.
     unsafe { libc::umask(mask as libc::mode_t) };
-    EnvStackSetResult::ENV_OK
+    EnvStackSetResult::Ok
 }
 
 /// A query for environment variables.
@@ -738,7 +738,7 @@ impl EnvStackImpl {
             flags.pathvar = Some(query.pathvar);
         }
 
-        let mut result = ModResult::new(EnvStackSetResult::ENV_OK);
+        let mut result = ModResult::new(EnvStackSetResult::Ok);
         if query.has_scope {
             // The user requested a particular scope.
             // If we don't have uvars, fall back to using globals.
@@ -799,26 +799,26 @@ impl EnvStackImpl {
         let query = Query::new(mode);
         // Users can't remove read-only keys.
         if query.user && is_read_only(key) {
-            return ModResult::new(EnvStackSetResult::ENV_SCOPE);
+            return ModResult::new(EnvStackSetResult::Scope);
         }
 
         // Helper to invoke remove_from_chain and map a false return to not found.
         fn remove_from_chain(node: &mut EnvNodeRef, key: &wstr) -> EnvStackSetResult {
             if EnvStackImpl::remove_from_chain(node, key) {
-                EnvStackSetResult::ENV_OK
+                EnvStackSetResult::Ok
             } else {
-                EnvStackSetResult::ENV_NOT_FOUND
+                EnvStackSetResult::NotFound
             }
         }
 
-        let mut result = ModResult::new(EnvStackSetResult::ENV_OK);
+        let mut result = ModResult::new(EnvStackSetResult::Ok);
         if query.has_scope {
             // The user requested erasing from a particular scope.
             if query.universal {
                 if uvars().remove(key) {
-                    result.status = EnvStackSetResult::ENV_OK;
+                    result.status = EnvStackSetResult::Ok;
                 } else {
-                    result.status = EnvStackSetResult::ENV_NOT_FOUND;
+                    result.status = EnvStackSetResult::NotFound;
                 }
                 // Note we have historically set this even if the uvar is not found.
                 result.uvar_modified = true;
@@ -846,7 +846,7 @@ impl EnvStackImpl {
         } else if uvars().remove(key) {
             result.uvar_modified = true;
         } else {
-            result.status = EnvStackSetResult::ENV_NOT_FOUND;
+            result.status = EnvStackSetResult::NotFound;
         }
         result
     }
@@ -937,12 +937,12 @@ impl EnvStackImpl {
 
         // If a variable is electric, it may only be set in the global scope.
         if query.has_scope && !query.global {
-            return Some(EnvStackSetResult::ENV_SCOPE);
+            return Some(EnvStackSetResult::Scope);
         }
 
         // If the variable is read-only, the user may not set it.
         if query.user && ev.readonly() {
-            return Some(EnvStackSetResult::ENV_PERM);
+            return Some(EnvStackSetResult::Perm);
         }
 
         // Be picky about exporting.
@@ -953,7 +953,7 @@ impl EnvStackImpl {
                 query.unexports
             };
             if !matches {
-                return Some(EnvStackSetResult::ENV_SCOPE);
+                return Some(EnvStackSetResult::Scope);
             }
         }
 
@@ -967,7 +967,7 @@ impl EnvStackImpl {
                 self.base.perproc_data.pwd = pwd;
                 self.base.globals.borrow_mut().changed_exported();
             }
-            return Some(EnvStackSetResult::ENV_OK);
+            return Some(EnvStackSetResult::Ok);
         }
         // Claim the value.
         let val = std::mem::take(val);
@@ -979,7 +979,7 @@ impl EnvStackImpl {
             pathvar: Some(false),
         };
         Self::set_in_node(&mut self.base.globals, key, val, flags);
-        return Some(EnvStackSetResult::ENV_OK);
+        return Some(EnvStackSetResult::Ok);
     }
 
     /// Set a universal variable, inheriting as applicable from the given old variable.

src/parse_execution.rs

@@ -990,7 +990,7 @@ impl<'a> ParseExecutionContext {
             EnvMode::LOCAL | EnvMode::USER,
             var.map_or(vec![], |var| var.as_list().to_owned()),
         );
-        assert!(retval == EnvStackSetResult::ENV_OK);
+        assert!(retval == EnvStackSetResult::Ok);
 
         trace_if_enabled_with_args(ctx.parser(), L!("for"), &arguments);
 
@@ -1010,7 +1010,7 @@ impl<'a> ParseExecutionContext {
                 .vars()
                 .set(&for_var_name, EnvMode::USER, vec![val]);
             assert!(
-                retval == EnvStackSetResult::ENV_OK,
+                retval == EnvStackSetResult::Ok,
                 "for loop variable should have been successfully set"
             );
             event::fire(ctx.parser(), evt.clone());

src/parser.rs

@@ -847,7 +847,6 @@ impl Parser {
     }
 
     /// Cover of vars().set(), which also fires any returned event handlers.
-    /// Return a value like ENV_OK.
     pub fn set_var_and_fire(
         &self,
         key: &wstr,
@@ -855,7 +854,7 @@ impl Parser {
         vals: Vec<WString>,
     ) -> EnvStackSetResult {
         let res = self.vars().set(key, mode, vals);
-        if res == EnvStackSetResult::ENV_OK {
+        if res == EnvStackSetResult::Ok {
             event::fire(self, Event::variable_set(key.to_owned()));
         }
         res

src/tests/expand.rs

@@ -361,7 +361,7 @@ fn test_expand_overflow() {
     let parser = Parser::principal_parser().shared();
     parser.vars().push(true);
     let set = parser.vars().set(L!("bigvar"), EnvMode::LOCAL, vals);
-    assert_eq!(set, EnvStackSetResult::ENV_OK);
+    assert_eq!(set, EnvStackSetResult::Ok);
 
     let mut errors = ParseErrorList::new();
     let ctx =