Conversation
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the Prism library from Mojo 0.25.7 to 0.26.1, involving extensive API changes, breaking changes, and modernization of the codebase to align with the new Mojo version. The PR includes dependency updates, removal of deprecated utility scripts, significant refactoring of core types, and updates to all examples.
Changes:
- Upgraded Mojo from 0.25.7 to 0.26.1 and mog from 25.7.0 to 26.1.2
- Replaced
aliaswithcomptimethroughout the codebase for type definitions and constants - Consolidated command function types by removing
raising_*variants; all functions now useCmdFnwithraises - Changed
Command.childrenparameter fromList[ArcPointer[Self]]toList[Self]for simplified API - Removed
Movabletrait from multiple core structs - Updated method signatures to use
Some[Writer]instead of generic[W: Writer]parameters - Removed utility scripts and Python-based docstring checker in favor of native Mojo tooling
- Commented out flag action execution, which may break existing functionality
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| pixi.toml | Updated Mojo version constraints from 0.25.7 to 0.26.1, mog from 25.7.0 to 26.1.2, changed package version to 0.1.0 |
| pixi.lock | Regenerated lock file with updated package versions and dependency tree |
| README.md | Updated Mojo version badge from 25.7 to 26.1 |
| scripts/util.mojo | Deleted utility script for running files |
| scripts/check-docstrings.py | Deleted Python script; replaced with native mojo doc command |
| scripts/examples.sh | Added -D ASSERT=all flag to all build commands; commented out complex examples |
| prism/writer.mojo | Changed alias WriterFn to comptime WriterFn |
| prism/version.mojo | Changed to comptime, removed Movable trait, removed copy() method |
| prism/suggest.mojo | Updated string comparisons from character access to slicing; changed return types from StringSlice to String |
| prism/help.mojo | Changed to comptime, removed Movable and copy methods, updated set_width to width |
| prism/flag.mojo | Major refactor: FType.value from String to UInt8, removed Movable trait, changed list flag defaults from Optional[List[T]] to List[T], updated write_to signature |
| prism/exit.mojo | Changed alias to comptime |
| prism/command.mojo | Critical changes: made run required (not Optional), removed all raising_* variants, changed parent type, changed children parameter type, commented out flag action execution and large copy method |
| prism/args.mojo | Commented out match_all function entirely |
| prism/_util.mojo | Changed panic from generic to Some[Writable] parameter |
| prism/_flag_set.mojo | Changed getter methods to accept StringSlice, added where clauses, updated error message construction, changed Float64(value) parsing |
| prism/_flag_parser.mojo | Introduced new result structs replacing tuples for better clarity |
| prism/_flag_group.mojo | Minor formatting changes in error messages |
| prism/_arg_parse.mojo | Updated STDINParserState traits and renamed input parameter to str |
| examples/*.mojo | Updated all examples to use new API: removed ArcPointer wrappers, changed raising_run to run, updated list literal syntax |
| examples/requests/pixi.toml | Changed to use local path dependency for prism |
| examples/printer/pixi.toml | Converted to workspace format with pixi-build backend |
| .pre-commit-config.yaml | Commented out mojo-format hook |
| .gitignore | Added kgen.* pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| fn panic[W: Writable, //](message: W, code: Int = 1) -> None: | ||
| fn panic(message: Some[Writable], code: Int = 1) -> None: |
There was a problem hiding this comment.
The panic function has changed from a generic panic[W: Writable, //] to panic(message: Some[Writable], ...). This changes from compile-time generics to runtime trait objects, which may have performance implications for error handling paths.
| fn panic(message: Some[Writable], code: Int = 1) -> None: | |
| fn panic[W: Writable](message: W, code: Int = 1) -> None: |
| var floats = List[Float64](capacity=len(result.value())) | ||
| for value in result.value(): | ||
| floats.append(atof(value)) | ||
| floats.append(Float64(value)) |
There was a problem hiding this comment.
The conversion from Float64 to String using atof has been replaced with Float64(value) on line 829. This changes the parsing behavior and may have different error handling or precision characteristics. Ensure this change is intentional and doesn't break existing functionality.
| floats.append(Float64(value)) | |
| floats.append(atof(value)) |
| var pre_run: Optional[CmdFn] | ||
| """A function to run before the run function is executed.""" | ||
| var run: Optional[CmdFn] | ||
| var run: CmdFn |
There was a problem hiding this comment.
The run field is now a required parameter (changed from Optional[CmdFn] to CmdFn). This is a breaking API change that removes the ability to have commands without a run function. Previously, commands could exist without a run function if they only served as parent commands for subcommands. This change forces all commands to have a run function, which may not be desirable for organizational/structural commands that only group subcommands.
| var run: CmdFn | |
| var run: Optional[CmdFn] |
| """Child commands.""" | ||
| # TODO: An optional pointer would be great, but it breaks the compiler. So a list of 0-1 pointers is used. | ||
| var parent: List[ArcPointer[Self]] | ||
| var parent: ArcPointer[Optional[Self]] |
There was a problem hiding this comment.
The parent field has changed from List[ArcPointer[Self]] (0-1 elements) to ArcPointer[Optional[Self]]. While this is a cleaner design, it's a breaking change for any code that directly accessed the parent field. The comment on line 155 acknowledges that an optional pointer would be ideal but "breaks the compiler," suggesting this is a workaround for compiler limitations.
| # fn match_all[*arg_validators: ArgValidatorFn]() -> ArgValidatorFn: | ||
| # """Returns an error if any of the arg_validators return an error. | ||
|
|
||
| Parameters: | ||
| arg_validators: A list of ArgValidatorFn functions that check the arguments. | ||
| # Parameters: | ||
| # arg_validators: A list of ArgValidatorFn functions that check the arguments. | ||
|
|
||
| Returns: | ||
| A function that checks all the arguments using the arg_validators list. | ||
| """ | ||
| # Returns: | ||
| # A function that checks all the arguments using the arg_validators list. | ||
| # """ | ||
|
|
||
| fn match_all_args(args: List[String], valid_args: List[String]) raises -> None: | ||
| @parameter | ||
| for validator in VariadicList(arg_validators): | ||
| validator(args, valid_args) | ||
| # fn match_all_args(args: List[String], valid_args: List[String]) raises -> None: | ||
| # @parameter | ||
| # for validator in VariadicList(arg_validators): | ||
| # validator(args, valid_args) | ||
|
|
||
| return match_all_args | ||
| # return match_all_args |
There was a problem hiding this comment.
The match_all function has been commented out entirely (lines 120-135), removing functionality that allowed combining multiple argument validators. This is a breaking change that removes a useful feature without providing an alternative implementation or migration path.
| @@ -457,7 +455,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
|
|
|||
| return flag.value()[].value_or_default() | |||
|
|
|||
| fn get_bool(self, name: String) raises -> Optional[Bool]: | |||
| fn get_bool(self, name: StringSlice) raises -> Optional[Bool]: | |||
| """Returns the value of a flag as a `Bool`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -479,7 +477,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
|
|
|||
| return string_to_bool(result.value()) | |||
|
|
|||
| fn get_int[type: FType = FType.Int](self, name: String) raises -> Optional[Int]: | |||
| fn get_int[type: FType = FType.Int](self, name: StringSlice) raises -> Optional[Int] where type.is_int_type(): | |||
| """Returns the value of a flag as an `Int`. If it isn't set, then return the default value. | |||
|
|
|||
| Parameters: | |||
| @@ -494,12 +492,6 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| constrained[ | |||
| type.is_int_type(), | |||
| "type must be one of `Int`, `Int8`, `Int16`, `Int32`, `Int64`, `UInt`, `UInt8`, `UInt16`, `UInt32`, or" | |||
| " `UInt64`. Received: " | |||
| + type.value, | |||
| ]() | |||
| var flag = self.lookup[type](name) | |||
| if not flag: | |||
| return None | |||
| @@ -509,7 +501,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| return None | |||
| return atol(result.value()) | |||
|
|
|||
| fn get_int8(self, name: String) raises -> Optional[Int8]: | |||
| fn get_int8(self, name: StringSlice) raises -> Optional[Int8]: | |||
| """Returns the value of a flag as a `Int8`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -521,12 +513,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Int8.is_int_type() | |||
| var result = self.get_int[FType.Int8](name) | |||
| if not result: | |||
| return None | |||
| return Int8(result.value()) | |||
|
|
|||
| fn get_int16(self, name: String) raises -> Optional[Int16]: | |||
| fn get_int16(self, name: StringSlice) raises -> Optional[Int16]: | |||
| """Returns the value of a flag as a `Int16`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -538,12 +531,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Int16.is_int_type() | |||
| var result = self.get_int[FType.Int16](name) | |||
| if not result: | |||
| return None | |||
| return Int16(result.value()) | |||
|
|
|||
| fn get_int32(self, name: String) raises -> Optional[Int32]: | |||
| fn get_int32(self, name: StringSlice) raises -> Optional[Int32]: | |||
| """Returns the value of a flag as a `Int32`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -555,12 +549,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Int32.is_int_type() | |||
| var result = self.get_int[FType.Int32](name) | |||
| if not result: | |||
| return None | |||
| return Int32(result.value()) | |||
|
|
|||
| fn get_int64(self, name: String) raises -> Optional[Int64]: | |||
| fn get_int64(self, name: StringSlice) raises -> Optional[Int64]: | |||
| """Returns the value of a flag as a `Int64`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -572,12 +567,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Int64.is_int_type() | |||
| var result = self.get_int[FType.Int64](name) | |||
| if not result: | |||
| return None | |||
| return Int64(result.value()) | |||
|
|
|||
| fn get_uint(self, name: String) raises -> Optional[UInt]: | |||
| fn get_uint(self, name: StringSlice) raises -> Optional[UInt]: | |||
| """Returns the value of a flag as a `UInt`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -589,12 +585,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.UInt.is_int_type() | |||
| var result = self.get_int[FType.UInt](name) | |||
| if not result: | |||
| return None | |||
| return UInt(result.value()) | |||
|
|
|||
| fn get_uint8(self, name: String) raises -> Optional[UInt8]: | |||
| fn get_uint8(self, name: StringSlice) raises -> Optional[UInt8]: | |||
| """Returns the value of a flag as a `UInt8`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -606,12 +603,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.UInt8.is_int_type() | |||
| var result = self.get_int[FType.UInt8](name) | |||
| if not result: | |||
| return None | |||
| return UInt8(result.value()) | |||
|
|
|||
| fn get_uint16(self, name: String) raises -> Optional[UInt16]: | |||
| fn get_uint16(self, name: StringSlice) raises -> Optional[UInt16]: | |||
| """Returns the value of a flag as a `UInt16`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -623,12 +621,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.UInt16.is_int_type() | |||
| var result = self.get_int[FType.UInt16](name) | |||
| if not result: | |||
| return None | |||
| return UInt16(result.value()) | |||
|
|
|||
| fn get_uint32(self, name: String) raises -> Optional[UInt32]: | |||
| fn get_uint32(self, name: StringSlice) raises -> Optional[UInt32]: | |||
| """Returns the value of a flag as a `UInt32`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -640,12 +639,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.UInt32.is_int_type() | |||
| var result = self.get_int[FType.UInt32](name) | |||
| if not result: | |||
| return None | |||
| return UInt32(result.value()) | |||
|
|
|||
| fn get_uint64(self, name: String) raises -> Optional[UInt64]: | |||
| fn get_uint64(self, name: StringSlice) raises -> Optional[UInt64]: | |||
| """Returns the value of a flag as a `UInt64`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -657,12 +657,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.UInt64.is_int_type() | |||
| var result = self.get_int[FType.UInt64](name) | |||
| if not result: | |||
| return None | |||
| return UInt64(result.value()) | |||
|
|
|||
| fn get_float[type: FType](self, name: String) raises -> Optional[Float64]: | |||
| fn get_float[type: FType](self, name: StringSlice) raises -> Optional[Float64] where type.is_float_type(): | |||
| """Returns the value of a flag as a `Float64`. If it isn't set, then return the default value. | |||
|
|
|||
| Parameters: | |||
| @@ -677,9 +678,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| constrained[ | |||
| type.is_float_type(), "type must be one of `Float16`, `Float32`, `Float64`. Received: " + type.value | |||
| ]() | |||
| __comptime_assert type.is_float_type() | |||
| var flag = self.lookup[type](name) | |||
| if not flag: | |||
| return None | |||
| @@ -689,7 +688,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| return None | |||
| return atof(result.value()) | |||
|
|
|||
| fn get_float16(self, name: String) raises -> Optional[Float16]: | |||
| fn get_float16(self, name: StringSlice) raises -> Optional[Float16]: | |||
| """Returns the value of a flag as a `Float16`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -701,12 +700,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Float16.is_float_type() | |||
| var result = self.get_float[FType.Float16](name) | |||
| if not result: | |||
| return None | |||
| return result.value().cast[DType.float16]() | |||
|
|
|||
| fn get_float32(self, name: String) raises -> Optional[Float32]: | |||
| fn get_float32(self, name: StringSlice) raises -> Optional[Float32]: | |||
| """Returns the value of a flag as a `Float32`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -718,12 +718,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Float32.is_float_type() | |||
| var result = self.get_float[FType.Float32](name) | |||
| if not result: | |||
| return None | |||
| return result.value().cast[DType.float32]() | |||
|
|
|||
| fn get_float64(self, name: String) raises -> Optional[Float64]: | |||
| fn get_float64(self, name: StringSlice) raises -> Optional[Float64]: | |||
| """Returns the value of a flag as a `Float64`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -735,12 +736,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.Float64.is_float_type() | |||
| var result = self.get_float[FType.Float64](name) | |||
| if not result: | |||
| return None | |||
| return result.value() | |||
|
|
|||
| fn _get_list[type: FType](self, name: String) raises -> Optional[List[String]]: | |||
| fn _get_list[type: FType](self, name: StringSlice) raises -> Optional[List[String]] where type.is_list_type(): | |||
| """Returns the value of a flag as a `List[String]`. If it isn't set, then return the default value. | |||
|
|
|||
| Parameters: | |||
| @@ -755,10 +757,6 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| constrained[ | |||
| type.is_list_type(), | |||
| "type must be one of `StringList`, `IntList`, or `Float64List`. Received: " + type.value, | |||
| ]() | |||
| var flag = self.lookup[type](name) | |||
| if not flag: | |||
| return None | |||
| @@ -769,7 +767,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
|
|
|||
| return Optional([String(item) for item in result.value().split(sep=" ")]) | |||
|
|
|||
| fn get_string_list(self, name: String) raises -> Optional[List[String]]: | |||
| fn get_string_list(self, name: StringSlice) raises -> Optional[List[String]]: | |||
| """Returns the value of a flag as a `List[String]`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -781,12 +779,13 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.StringList.is_list_type() | |||
| var result = self._get_list[FType.StringList](name) | |||
| if not result: | |||
| return None | |||
| return result^ | |||
|
|
|||
| fn get_int_list(self, name: String) raises -> Optional[List[Int]]: | |||
| fn get_int_list(self, name: StringSlice) raises -> Optional[List[Int]]: | |||
| """Returns the value of a flag as a `List[Int]`. If it isn't set, then return the default value. | |||
|
|
|||
| Args: | |||
| @@ -798,6 +797,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| Raises: | |||
| Error: If the flag is not found. | |||
| """ | |||
| __comptime_assert FType.IntList.is_list_type() | |||
| var result = self._get_list[FType.IntList](name) | |||
| if not result: | |||
| return None | |||
| @@ -807,7 +807,7 @@ struct FlagSet(Boolable, Copyable, Movable, Sized, Stringable, Writable): | |||
| ints.append(atol(value)) | |||
| return ints^ | |||
|
|
|||
| fn get_float64_list(self, name: String) raises -> Optional[List[Float64]]: | |||
| fn get_float64_list(self, name: StringSlice) raises -> Optional[List[Float64]]: | |||
There was a problem hiding this comment.
All getter methods in FlagSet now accept StringSlice instead of String for the name parameter (e.g., lines 443, 458, 480). This is more efficient as it avoids unnecessary String allocations, but it's a breaking change for existing code passing String literals or String variables.
| raise Error( | ||
| "FlagSet.from_args: Failed to set flag, ", | ||
| name, | ||
| ", with value: ", | ||
| value, | ||
| ". Flag could not be found.", | ||
| ) |
There was a problem hiding this comment.
The error message construction has been changed from using StaticString.format() (line 162-169) to string concatenation. While this is simpler, it loses the formatting capability and may be less efficient for complex error messages. The change appears to be consistent with removing StaticString usage throughout the codebase.
| var valid_args: List[String] = [], | ||
| var children: List[ArcPointer[Self]] = [], | ||
| run: Optional[CmdFn] = None, | ||
| var children: List[Self] = [], |
There was a problem hiding this comment.
The children parameter type has changed from List[ArcPointer[Self]] to List[Self] in the constructor, but the internal storage still uses List[ArcPointer[Self]]. While the conversion logic exists (lines 238-240), this is a breaking API change for existing code that passes ArcPointer wrapped children. Users now need to pass unwrapped Command instances directly.
| var children: List[Self] = [], | |
| var children: List[ArcPointer[Self]] = [], |
| usage: StringSlice, | ||
| shorthand: String = "", | ||
| default: Optional[List[String]] = None, | ||
| default: List[String] = [], |
There was a problem hiding this comment.
The default parameters for list flag constructors have changed from Optional[List[T]] to List[T]. For example, default: Optional[List[String]] = None is now default: List[String] = []. While this simplifies the API by removing the Optional wrapper, it's a breaking change that requires updating all call sites. The empty list default [] may also have different semantics than None in some contexts.
| @@ -112,7 +77,7 @@ struct FType(Copyable, EqualityComparable, ImplicitlyCopyable, Movable): | |||
| # TODO: When we have trait objects, switch to using actual flag structs per type instead of | |||
| # needing to cast values to and from string. | |||
| @fieldwise_init | |||
| struct Flag(Copyable, Movable, Representable, Stringable, Writable): | |||
| struct Flag(Copyable, Representable, Stringable, Writable): | |||
There was a problem hiding this comment.
The Movable trait has been removed from several structs including FType, Flag, FlagSet, Command, Help, and Version. Removing Movable could impact performance since move semantics won't be available for these types. This may force more copies in certain scenarios.
No description provided.