Conversation
BREAKING CHANGE: removed feature flags
BREAKING CHANGE: renamedApiError to Error and removed worldstate prelude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BREAKING CHANGE: .item fields changed to the new wrapper type
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BREAKING CHANGE: changed module tree structure to be more intuitive
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
I agree that O(n^2) does look bad, however, the collection has at most like ~10 items. I think constructing a HashSet might actually be slower in this case. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
devtools.nu (1)
18-28: Good implementation of a market API request function with language support.The function is well-structured with clear parameter documentation and proper handling of the optional language parameter. However, there are a few potential improvements:
- The base URL is repeated in both conditional branches
- There's no error handling for failed HTTP requests
Consider refactoring to avoid URL duplication:
export def market_req [ endpoint:string, # The endpoint to request language?:string@languages # The language to request the endpoint in ]: nothing -> table { + let base_url = "https://api.warframe.market/v2" if language == null { - return (http get $"https://api.warframe.market/v2($endpoint)") + return (http get $"($base_url)($endpoint)") } else { - return (http get $"https://api.warframe.market/v2($endpoint)" --headers [Language $language]) + return (http get $"($base_url)($endpoint)" --headers [Language $language]) } }src/market/models/mod.rs (1)
1-6: Module structure has been simplified and reorganized.The market models module structure has been reorganized to focus on the core components needed for the V7 implementation. The removal of previous modules (
item_info,orders, andstatistic_item) and addition of new ones (base,i18n,item_short,set_items) aligns with the PR objectives.However, the module doesn't re-export any types, which means users will need to import them directly from submodules.
Consider re-exporting commonly used types for easier access:
pub mod base; pub mod i18n; pub mod item; pub mod item_short; pub mod set_items; pub mod versions; + // Re-export commonly used types + pub use i18n::Language; + pub use item::Item; + pub use set_items::SetItems;src/market/cache.rs (2)
17-17: Consider adding documentation for the use ofArc<str>.While the comment explains the purpose of
path_data, it would be helpful to document whyArc<str>was chosen overString(for shared ownership with minimal overhead).
1-37: Consider adding accessors for the struct fields.The
CacheKeystruct doesn't provide any methods to access its fields after creation. Consider adding getter methods if other parts of the code need to inspect these values.+ pub fn get_type_id(&self) -> TypeId { + self.type_id + } + + pub fn get_language(&self) -> Language { + self.language + } + + pub fn get_path_data(&self) -> &str { + &self.path_data + }src/market/error.rs (1)
12-17: Reconsider theEmptyErrorAndDataerror variant messaging.The documentation comment states "should not happen though", which suggests this might be a programming error rather than an expected error condition. Consider either:
- Providing more context about when this might occur
- Making the error message more actionable if it does happen
Also, the error message could be more specific about what the user should do when encountering this error.
- #[error("API responded with both an empty error and empty data")] + #[error("API responded with both an empty error and empty data. This indicates a possible API issue or malformed response.")]src/market/mod.rs (1)
29-29: Add documentation for the BASE_URL constant.While the constant's purpose is relatively clear from its name, adding documentation would improve code clarity.
-pub const BASE_URL: &str = "https://api.warframe.market/v2"; +/// Base URL for the Warframe Market API v2. +/// This is used as the prefix for all API endpoints. +pub const BASE_URL: &str = "https://api.warframe.market/v2";src/market/models/base.rs (2)
47-57: URL duplication in the impl_endpoint macro.The macro hardcodes the base URL "https://api.warframe.market/v2" which duplicates the BASE_URL constant defined in mod.rs. This creates a maintenance issue if the URL ever changes.
macro_rules! impl_endpoint { ($name:ident, $endpoint:expr) => { impl crate::market::Endpoint for $name { fn endpoint() -> &'static str { - concat!("https://api.warframe.market/v2", $endpoint) + concat!(crate::market::BASE_URL, $endpoint) } } }; }To implement this fix, you'll need to ensure the BASE_URL is accessible at compile time for the macro.
24-44: Consider using async fn instead of returning impl Future.The query method returns an impl Future, but modern Rust supports async fn directly, which would make the code cleaner and more straightforward.
- #[must_use] - fn query( - client: &reqwest::Client, - language: Language, - ) -> impl Future<Output = Result<Self::Data, Error>> + Send { - async move { + #[must_use] + async fn query( + client: &reqwest::Client, + language: Language, + ) -> Result<Self::Data, Error> {This change would make the code more readable and maintain the same functionality.
src/market/client.rs (1)
100-109: Review cache type safety.Storing data as
Arc<dyn Any + Send + Sync>is flexible but bypasses static type checking. For large-scale usage, consider a strongly typed cache or use an enum to ensure compile-time guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Cargo.toml(2 hunks)devtools.nu(1 hunks)src/market/cache.rs(1 hunks)src/market/client.rs(1 hunks)src/market/error.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/base.rs(1 hunks)src/market/models/i18n.rs(1 hunks)src/market/models/item.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/set_items.rs(1 hunks)warframe-macros/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- warframe-macros/Cargo.toml
🧰 Additional context used
🧬 Code Definitions (2)
src/market/cache.rs (1)
src/market/client.rs (1)
new(58-73)
src/market/models/base.rs (2)
src/market/cache.rs (1)
language(30-36)src/market/client.rs (2)
response(195-195)response(236-236)
🔇 Additional comments (25)
devtools.nu (1)
1-16: Well-organized language constants for internationalization.The
languagesfunction provides a clean list of supported language codes that can be used throughout the application. The function signature is clear and the return type is properly specified.src/market/models/set_items.rs (1)
1-4: Appropriate imports for the SetItems struct.The imports are minimal and focused on what's needed for the struct definition.
src/market/models/i18n.rs (3)
3-8: Good use of derive-more for enhanced trait derivation.Using the derive-more crate to add Display and FromStr implementations is a clean approach to extending the enum's functionality.
9-38: Comprehensive Language enum with appropriate trait derivations.The Language enum is well-designed with:
- Proper serde rename attributes for serialization/deserialization
- Comprehensive trait derivations for various use cases
- A sensible default (English)
This will work well for internationalization purposes.
40-40: Flexible I18N type alias for internationalization data.The I18N type alias provides a reusable way to associate localized data with language codes using a HashMap. This is a good abstraction for handling translated content.
Consider adding a documentation comment to explain the purpose and usage of this type alias:
+/// A mapping of languages to localized content of type T. +/// Used to store internationalized data for various languages. pub type I18N<T> = HashMap<Language, T>;src/market/cache.rs (3)
8-18: Solid design for theCacheKeystruct with type-safety considerations.The struct is well-designed, utilizing
TypeIdfor type differentiation and usingArc<str>for efficient string sharing. The derive implementations are appropriate for a cache key.
22-28: LGTM! Thenewmethod is well-implemented.The implementation correctly initializes all fields and properly converts the
path_datastring slice to anArc<str>using theinto()method.
30-36: LGTM! Thelanguagemethod provides a convenient shorthand.This is a useful convenience method for when you only need to differentiate by type and language without additional path data.
src/market/error.rs (2)
3-18: Well-designed error enum using thiserror.The Error enum makes good use of the thiserror crate for better error handling. The transparent attribute ensures that inner errors are properly displayed. The variant documentation is clear and helpful.
20-20: LGTM! Good use of type alias for Result.Using a type alias for the Result type is a good practice that will reduce boilerplate throughout the codebase.
src/market/mod.rs (4)
1-4: Excellent documentation with API reference.The module documentation clearly explains its purpose and includes a link to the official API documentation, which is very helpful for maintainers.
4-8: Good encapsulation with appropriate visibility modifiers.Making the
cacheandmodelsmodulespub(crate)limits their visibility to just this crate, which is good encapsulation practice.
10-19: Well-organized public API exports.The explicit re-exports make it clear what's part of the public API, which improves usability.
21-27: Good organization of queryable types.Creating a dedicated submodule for queryable types helps organize related functionality.
src/market/models/base.rs (4)
9-15: Well-designed ResponseBase struct for API responses.The struct correctly captures the common structure of API responses with appropriate field names and types. Using Option for data and error fields ensures proper handling of null values.
17-19: Good abstraction with the Endpoint trait.The Endpoint trait provides a clean abstraction for specifying API endpoints.
21-45: Well-implemented Queryable trait with proper error handling.The Queryable trait is a good abstraction for types that can be queried from the API. The implementation correctly handles errors and performs appropriate JSON deserialization.
59-72: LGTM! Well-designed macros for trait implementation.These macros provide a convenient way to implement the traits for different data structures, reducing boilerplate code.
src/market/models/item.rs (2)
40-40: Verify necessity of ordering traits.You're deriving
PartialOrdandOrdforItemI18N. Confirm that these traits are actually needed. They can be removed if no ordering operation is performed onItemI18N.
61-68: Confirm existence and content of test fixture.The test references
"src/market/models/fixtures/item.json". Ensure this file exists, has valid JSON, and aligns with the newItemstruct fields.Cargo.toml (3)
4-4: Revisit Rust edition.The 2024 edition is still experimental. Same concern was raised previously.
11-11: Confirm that 1.85 is supported.
rust-version = "1.85"is quite forward-looking. Validate that users have or can install this compiler version.
15-50: Feature reorganization looks good.Switching the default feature to
"market_ratelimit"and introducing clearer feature sets help maintain modularity.src/market/client.rs (2)
131-139: Fetch methods are well-structured.The generic
fetchapproach, combined with language-based variants, neatly centralizes the logic. No issues spotted here.
254-295: Macros improve readability and avoid repetitive code.Using
ratelimit!,try_get_cache!, andinsert_cache!macros clarifies the flow. Great approach to reduce boilerplate.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/market/models/item_short.rs (1)
42-53: Consider adding Default implementationThe ItemShortI18N struct looks well-structured with appropriate derives. Since it represents a common pattern for localized data, consider implementing Default to simplify creation in tests and default cases.
#[derive(Debug, Deserialize, PartialEq, Eq, PartialOrd, Ord, Clone, Hash)] #[serde(rename_all = "camelCase")] +#[derive(Default)] pub struct ItemShortI18N { /// Localized name of the item pub name: String, /// Localized icon of the item pub icon: String, /// Localized thumbnail of the item pub thumb: String, /// Optional localized sub-icon of the item pub sub_icon: Option<String>, }src/market/mod.rs (1)
30-30: Consider using more specific constant nameWhile BASE_URL is descriptive, consider using a more specific name like MARKET_API_BASE_URL to avoid potential naming conflicts if other modules also define BASE_URL constants.
-pub const BASE_URL: &str = "https://api.warframe.market/v2"; +pub const MARKET_API_BASE_URL: &str = "https://api.warframe.market/v2";src/market/client.rs (1)
111-121: HTTP request
Method correctly awaits the response.
Consider explicit handling of other non-2xx statuses if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Cargo.toml(2 hunks)src/market/cache.rs(1 hunks)src/market/client.rs(1 hunks)src/market/error.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/item.rs(1 hunks)src/market/models/item_short.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/versions.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/market/models/versions.rs
🧰 Additional context used
🧬 Code Definitions (1)
src/market/client.rs (1)
src/market/models/mod.rs (2)
query(31-50)client(36-41)
🪛 GitHub Actions: CI
src/market/client.rs
[error] 237-237: cannot find type Arc in this scope. Consider importing this struct: use std::sync::Arc;
[error] 237-237: cannot find type ItemShort in this scope. Consider importing this struct through its public re-export: use crate::market::queryable::ItemShort;
[error] 246-246: cannot find type ItemShort in this scope. Consider importing this struct through its public re-export: use crate::market::queryable::ItemShort;
[error] 252-252: cannot find type Arc in this scope. Consider importing this struct: use std::sync::Arc;
[error] 252-252: cannot find type ItemShort in this scope. Consider importing this struct through its public re-export: use crate::market::queryable::ItemShort;
🔇 Additional comments (42)
src/market/cache.rs (2)
5-13: Well-structured CacheKey implementationThe CacheKey struct is well-designed with appropriate derived traits (Hash, PartialEq, Eq, etc.) that make it suitable for use as a key in hash-based collections. The documentation clearly explains the purpose of the endpoint field.
16-22: LGTM: Simple and effective constructorThe new method provides a clean interface for creating CacheKey instances, properly converting the string slice to Arc.
src/market/models/item_short.rs (2)
5-40: Well-documented struct with appropriate optionalityThe ItemShort struct is comprehensive and well-documented with clear comments explaining each field's purpose. The use of Option for optional fields is appropriate, and the serde configuration for camelCase naming follows API conventions.
55-70: Good test implementation with rstestThe test module is well-structured using rstest for data-driven testing. Using fixture files for JSON samples is a good practice that separates test data from test logic.
src/market/error.rs (2)
1-18: Improved error handling with thiserrorThe new Error enum is a significant improvement over the previous ApiError:
- Using thiserror simplifies error implementation
- The transparent error pattern for reqwest errors is clean
- The documentation for EmptyErrorAndData clearly explains the edge case
This change will make error handling more consistent and maintainable.
20-20: Helpful Result type aliasThe Result type alias simplifies function signatures throughout the codebase and follows Rust idioms for error handling.
src/market/mod.rs (4)
2-4: Improved module documentationThe added documentation comments clearly explain the module's purpose and provide a link to the API documentation, which is helpful for understanding the implementation.
4-8: Well-organized module structureThe conditional compilation for the cache module and the visibility restrictions on internal modules provide a clean public API surface while keeping implementation details private.
10-18: Clean public re-exportsRe-exporting key types at the module level follows Rust best practices, making the API more ergonomic by allowing users to import commonly used types directly from the market module.
20-28: Well-organized queryable moduleThe queryable submodule provides a clean namespace for all queryable types, making the API more discoverable and organized. This is a good architectural decision that separates interface from implementation.
src/market/models/item.rs (8)
1-3: Imports are consistent with usage
No issues found as the newly added imports (serde::Deserialize,super::i18n::I18N) match the subsequent usage.
5-7: Consider refactoring excessive booleansYou are already allowing
clippy::struct_excessive_bools, which indicates multiple boolean fields.
Consider an enum or composite structure for these fields to improve clarity, as suggested in an earlier review.
9-13: Fields look correct
The new fields appear well-defined and consistent with typical usage for an item.
No immediate concerns.
15-18: Nice usage of#[serde(default)]
This ensures thatset_partsdeserializes gracefully even if missing from the JSON.
20-25: Optional fields appear consistent
They handle absence gracefully, which is appropriate for the underlying data.
27-35: Handling numeric fields consistently
All fields are optional, which aligns with possible missing data.
37-38: Integration withI18N
No issues found.
40-48:ItemI18Nstruct
Using a dedicated struct for i18n fields helps keep the code organized for multilingual data. Looking good.src/market/models/mod.rs (4)
1-5: Modular reorganization
Introducing these new modules clarifies the codebase structure.
18-24: Check usage ofPartialEqandPartialOrd
Implementing these traits forResponseBase<T>can be beneficial for testing or sorting, but ensure thatTalso implements them where needed.Could you verify that
Tis guaranteed to bePartialOrdin all relevant usage scenarios?
26-50: Trait-based approach for market queries
TheQueryabletrait cleanly abstracts the API endpoint usage.
No issues found; the design looks consistent and flexible.
53-68: Macro for implementingQueryable
This approach reduces boilerplate for array vs. object-type data.
Looks well-structured and easy to extend.src/market/client.rs (13)
74-85: No concerns about the default constructor
Implementation is straightforward.
87-90: Builder pattern
No issues found. This approach provides flexible client configuration.
92-101: Async retrieval from cache
Implementation looks straightforward and leveragesdowncast_reffor type safety.
103-109: Cache insertion
Implementation is consistent withmoka::future::Cache.
123-129: Generic fetch for default language
Method is minimal, delegating tofetch_using_language. Looks good.
131-139: Fetch item by slug
No immediate issues, delegated tofetch_item_using_language.
141-158: Language-specific fetch
Using macros to handle caching and rate limiting is a neat approach.
160-193: Fetch item with language
Includes 404 handling that returnsOk(None).
Implementation is consistent with the approach used infetch_using_language.
195-231: Retrieve item sets
Implementation matches the pattern offetch_item_using_language.
Handling of 404 status is consistent.
264-272: Rate limiting macro
Implementation is straightforward.
273-283: Cache retrieval macro
Macro usage is well structured to handle optional vs. non-optional results.
299-307: Cache insertion macro
No issues found.
311-335: Cache equality test
This test ensures that consecutive calls for the same language return the same items, presumably from cache.
Consider verifying the actual data if needed.Cargo.toml (7)
4-4: Rust Edition VerificationThe
editionis now set to"2024". Please verify that using the 2024 edition is intentional and that it’s officially available given the context of your project. (Note: A similar concern was raised in previous reviews.)
11-11: MSRV Declaration AddedThe explicit declaration of
rust-version = "1.85"is a good addition—it improves clarity regarding the minimum supported Rust version. Ensure that your CI and local setups are aligned with this version.
15-21: Updated Feature ConfigurationThe features section has been refactored:
- The
defaultfeature now solely includes"market_ratelimit".- The
fullfeature now aggregates both"market_ratelimit"and"market_cache".- Additionally, the new features
"market_ratelimit"(depending on"dep:governor") and"market_cache"(depending on"dep:moka") are introduced.These changes appear to align with your restructured dependency requirements. Please double-check that these feature adjustments correctly correspond to conditional compilation paths in your code.
30-39: Dependency Updates and AdditionsSeveral dependency updates and new introductions are present:
- Upgraded
thiserrorto"2.0.11". Since this is a major version bump from previous versions, please verify that your error-handling code is compatible with any API changes.- Updated
serde_withto"3.11.0".- Newly added dependencies such as
warframe-macros,paste,tracing,governor, andderive_buildershould be reviewed to ensure they integrate seamlessly into your project.It would be beneficial to confirm that these changes (especially version upgrades) have been validated with your existing codebase and that any potential breaking changes are addressed.
41-43: Dev-Dependencies EnhancementThe introduction of
[dev-dependencies]withrstestandtracing-subscriberstrengthens your testing and logging setup. Make sure these additions are integrated into your CI and test frameworks.
45-46: Clippy Lints ActivationThe
[lints.clippy]section now enforcespedantic = "warn", which is a positive step towards maintaining high code quality. Just ensure that any resulting warnings are addressed appropriately across the codebase.
48-50: Workspace Configuration CheckThe new workspace configuration—with
resolver = "3"andmembers = ["warframe-macros", "."]—helps in managing dependencies and project structure efficiently. Please verify that all intended packages are correctly included as workspace members.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/market/client.rs (1)
3-38:⚠️ Potential issueImports need to be unconditionally available
The imports for
ArcandItemShortare still conditionally imported under#[cfg(feature = "market_cache")], but they're used in methods that are not conditional (likeitems()at line 248). This will cause compilation errors when themarket_cachefeature is disabled.Apply this fix to ensure
ArcandItemShortare always available:-#[cfg(feature = "market_cache")] -use { - super::cache::CacheKey, - super::queryable::ItemShort, - moka::future::Cache, - std::{ - any::Any, - sync::Arc, - time::Duration, - }, -}; +use super::cache::CacheKey; +use super::queryable::ItemShort; +use moka::future::Cache; +use std::{ + any::Any, + sync::Arc, + time::Duration, +};
🧹 Nitpick comments (5)
src/market/client.rs (5)
51-53: Avoid unwrap() in builder defaultsUsing
unwrap()in a builder's default value could cause a panic, even though it's unlikely in this case.- default = DefaultDirectRateLimiter::direct(Quota::per_second( - NonZeroU32::new(3).unwrap(), - )) + default = DefaultDirectRateLimiter::direct(Quota::per_second( + NonZeroU32::try_from(3).expect("3 is not zero") + ))Or better, define a constant:
+const DEFAULT_REQUESTS_PER_SECOND: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(3) }; - default = DefaultDirectRateLimiter::direct(Quota::per_second( - NonZeroU32::new(3).unwrap(), - )) + default = DefaultDirectRateLimiter::direct(Quota::per_second(DEFAULT_REQUESTS_PER_SECOND))
103-120: Missing documentation for cache helper methodsThese helper methods lack documentation explaining their purpose and behavior.
Add documentation to describe these methods:
#[cfg(feature = "market_cache")] + /// Retrieves a value from the cache by its key. + /// + /// Returns `None` if the key doesn't exist or if the value can't be cast to the requested type. async fn get_from_cache<T>(&self, key: CacheKey) -> Option<T> where T: 'static + Send + Sync + Clone, { // ... } #[cfg(feature = "market_cache")] + /// Inserts a value into the cache. + /// + /// Overwrites any existing value for the given key. async fn insert_into_cache<T>(&self, key: CacheKey, data: T) where T: 'static + Send + Sync + Clone, { // ... }
323-323: Remove unnecessary macro importMacros don't need to be imported with
usestatements.-use ratelimit;
349-349: Remove unnecessary macro importMacros don't need to be imported with
usestatements.-use try_get_cache;
361-361: Remove unnecessary macro importMacros don't need to be imported with
usestatements.-use insert_cache;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/market/client.rs(1 hunks)
🔇 Additional comments (11)
Cargo.toml (7)
4-4: Edition Field Potential Issue.
The edition is set to"2024", which might not be officially released yet. This was flagged in previous reviews; please verify if you intended to use the unreleased edition or revert to"2021".
11-11: Rust-Version Field Added.
Addingrust-version = "1.85"clearly specifies the minimum supported Rust version and is a good practice. Ensure that all code in the codebase is compliant with Rust 1.85.
14-21: Updated Feature Set.
The default and full features have been updated to include"market_ratelimit"and"market_cache", and new features have been introduced:
market_ratelimit = ["dep:governor"]market_cache = ["dep:moka"]
Please verify that these changes align with the intended module functionality (especially regarding market rate limiting and caching) and that any dependent code has been updated accordingly.
30-30: Dependency Update:thiserror.
Upgradingthiserrorfrom version"1.0.63"to"2.0.11"is a significant change that may introduce breaking modifications in error handling implementations. Confirm that all custom error types and usages are adjusted for the new version’s API.
34-39: New Dependencies Introduced.
New or updated dependencies have been added:
serde_withnow defined in table form, which enhances readability.warframe-macros,paste, andtracingintroduce additional macros and logging capabilities.governor(optional) andderive_builderhave also been introduced.Ensure that these dependencies are correctly configured and integrated into the overall project architecture.
41-44: Dev-Dependencies Added.
The additions ofrstestandtracing-subscriberunder[dev-dependencies]are appropriate for enhancing testing and logging during development. Verify that your test suite leverages these tools where applicable.
45-51: Clippy Lints and Workspace Configuration.
The active[lints.clippy]section withpedantic = "warn"and the new[workspace]configuration demonstrate a commitment to code quality and consistency across packages. Confirm that all workspace members (in this case,warframe-macrosand the current package) maintain consistent linting standards.src/market/client.rs (4)
42-83: Good use of builder patternThe
derive_builderapproach creates a clean API for configuring the client with reasonable defaults.
134-169: Well-implemented generic fetch method with cachingThis approach allows for type-safe requests with automatic caching and properly handles errors.
218-242: Good implementation of set_items_of with proper error handlingThe method properly handles:
- Cache hits/misses
- Rate limiting
- 404 responses
- Error responses
- Empty data responses
316-361: Well-designed macros for code reuseThese macros effectively reduce code duplication while maintaining readable error handling patterns.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/worldstate/models/steel_path.rs (1)
27-52: Consider enhancing test validationThe tests verify that JSON deserialization succeeds but don't validate the deserialized data's content. While this basic validation is useful, consider adding assertions to verify field values match expected values from the fixtures.
#[rstest] fn test(steel_path_en: &str) { - from_str::<R>(steel_path_en).unwrap(); + let steel_path = from_str::<R>(steel_path_en).unwrap(); + // Add assertions to verify field values + // For example: assert_eq!(steel_path.current_offer.name, "Expected Name"); }devtools.nu (2)
18-28: Consider consistent language handling between functions.The
market_reqfunction is well-implemented with proper parameter validation using@languages, but there's an inconsistency in how the default language is handled:
- When language is null, you add "?language=en" to the URL
- When language is specified, you use a header instead of a URL parameter
This might lead to confusion. Consider using the same approach (either headers or URL parameters) for both cases.
export def market_req [ endpoint:string, # The endpoint to request language?:string@languages # The language to request the endpoint in ]: nothing -> table { if language == null { - return (http get $"https://api.warframe.market/v2($endpoint)?language=en") + return (http get $"https://api.warframe.market/v2($endpoint)" --headers [Language "en"]) } else { return (http get $"https://api.warframe.market/v2($endpoint)" --headers [Language $language]) } }
1-40: Consider adding error handling and URL path validation.The functions currently lack error handling for HTTP requests and validation for the endpoint parameter. It would be beneficial to:
- Check if the endpoint starts with a slash and handle it appropriately
- Implement error handling for API requests
- Add more comprehensive documentation with examples
Here's an example implementation for the first point:
export def market_req [ endpoint:string, # The endpoint to request language?:string@languages # The language to request the endpoint in ]: nothing -> table { + # Ensure endpoint starts with a slash + let normalized_endpoint = if $endpoint starts-with "/" { $endpoint } else { "/" + $endpoint } + if language == null { - return (http get $"https://api.warframe.market/v2($endpoint)?language=en") + return (http get $"https://api.warframe.market/v2($normalized_endpoint)?language=en") } else { - return (http get $"https://api.warframe.market/v2($endpoint)" --headers [Language $language]) + return (http get $"https://api.warframe.market/v2($normalized_endpoint)" --headers [Language $language]) } }warframe-macros/src/model/struct_impl/return_style.rs (1)
15-37: Consider adding case insensitivity to the ReturnStyle parserThe current implementation is case-sensitive, requiring exact matches for "Object" and "Array". Consider making this case-insensitive for a more flexible API.
match ident.to_string().as_str() { "Object" => Ok(Self::Object), "Array" => Ok(Self::Array), + "object" => Ok(Self::Object), + "array" => Ok(Self::Array), _ => Err(syn::Error::new_spanned( ident, - "expected `Object` or `Array`", + "expected `Object` or `Array` (case sensitive)", )), }Alternatively, for a more robust solution:
- match ident.to_string().as_str() { - "Object" => Ok(Self::Object), - "Array" => Ok(Self::Array), - _ => Err(syn::Error::new_spanned( - ident, - "expected `Object` or `Array`", - )), - } + match ident.to_string().to_lowercase().as_str() { + "object" => Ok(Self::Object), + "array" => Ok(Self::Array), + _ => Err(syn::Error::new_spanned( + ident, + "expected `Object` or `Array` (case insensitive)", + )), + }src/worldstate/fixtures/deep_archimedea.rs (2)
1-188: Fix typo in fixture dataThere's a typo in the JSON data on line 140 (and the duplicate at 140): "Slain enemies burts with Void energy" should be "Slain enemies burst with Void energy".
- "description": "Slain enemies burts with Void energy." + "description": "Slain enemies burst with Void energy."
3-188: Consider adopting a DRY approach for duplicate JSON contentThe fixture contains identical JSON data duplicated in two sections (lines 6-94 and 98-186). Consider refactoring the
fixture!macro to avoid this duplication if possible, or adding a comment explaining why duplication is necessary.src/worldstate/models/deep_archimedia.rs (2)
27-65: Document the plan for commented-out codeThe commented-out
Deviationenum takes up significant space without being used. Consider either:
- Removing it entirely if it's not needed
- Adding a more detailed comment explaining future plans for this enum
- Opening a GitHub issue to track the implementation and reference it in the comment
This would improve code clarity and ensure the commented code isn't forgotten.
67-92: Consider adding more comprehensive testsThe current tests only verify that the JSON can be deserialized but don't validate that the deserialized objects have the expected values. Consider adding assertions that check specific fields to ensure the parsing logic is correct.
#[rstest] fn test(deep_archimedea_en: &str) { - from_str::<R>(deep_archimedea_en).unwrap(); + let result = from_str::<R>(deep_archimedea_en).unwrap(); + assert_eq!(result.id, "1743379200000DeepArchimedea"); + assert_eq!(result.missions.len(), 3); + assert_eq!(result.missions[0].r#type, MissionType::Extermination); + assert_eq!(result.personal_modifiers.len(), 4); }warframe-macros/src/model/struct_impl/args.rs (1)
35-57: Hardcodedpcplatform endpoint.Currently, the endpoint is fixed to
"https://api.warframestat.us/pc". If multi-platform support is needed (e.g.,ps4,xbox), consider parameterizing the base URL or providing an enum to represent platforms.src/worldstate/client.rs (1)
349-389: Similar concurrency concern applies to these multiple listener methods.Each method (
call_on_nested_update,call_on_update_with_state, andcall_on_nested_update_with_state) relies on a 30-second sleep loop and.to_std().unwrap(). Ensure that negative intervals do not cause panics in unusual conditions.You might catch or clamp negative durations in each listener method:
- tokio::time::sleep(time_to_sleep.to_std().unwrap()).await; + match time_to_sleep.to_std() { + Ok(duration) => tokio::time::sleep(duration).await, + Err(_) => { + // handle negative durations, e.g., skip or re-fetch immediately + continue; + } + }Also applies to: 447-487, 550-591
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
devtools.nu(1 hunks)src/worldstate/client.rs(14 hunks)src/worldstate/fixtures/deep_archimedea.rs(1 hunks)src/worldstate/fixtures/mod.rs(1 hunks)src/worldstate/mod.rs(1 hunks)src/worldstate/models/damage_type.rs(3 hunks)src/worldstate/models/deep_archimedia.rs(1 hunks)src/worldstate/models/mod.rs(1 hunks)src/worldstate/models/steel_path.rs(1 hunks)warframe-macros/src/lib.rs(1 hunks)warframe-macros/src/model/struct_impl/args.rs(1 hunks)warframe-macros/src/model/struct_impl/return_style.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/worldstate/models/damage_type.rs
🧰 Additional context used
🧬 Code Definitions (2)
warframe-macros/src/lib.rs (1)
warframe-macros/src/model/mod.rs (2)
syn(13-13)expand(12-21)
src/worldstate/models/steel_path.rs (5)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/news.rs (2)
test(58-60)test_ml(63-65)src/worldstate/models/sortie.rs (2)
test(56-58)test_ml(61-63)src/worldstate/models/alert.rs (2)
test(38-40)test_ml(43-45)
🔇 Additional comments (35)
src/worldstate/models/steel_path.rs (2)
3-11: Well-structured model with clear documentationThe
SteelPathShopItemstruct is well-defined with appropriate documentation for both the struct and its fields, making the code self-explanatory. The use of the#[model]attribute from the newwarframe-macroscrate aligns with the overall refactoring approach mentioned in the PR summary.
13-25: Good use of serde attributes for JSON mappingThe
SteelPathstruct uses appropriate model attributes to specify API endpoint details and includes the necessary#[serde(rename)]attribute to handle the JSON field naming difference. The documentation is clear and the struct fields are logically organized.devtools.nu (1)
1-16: Well-structured language function.The
languagesfunction provides a clear list of supported language codes and is properly exported with appropriate typing. The return typelist<string>is correctly defined.warframe-macros/src/lib.rs (1)
1-23: Well-structured procedural macro implementationThe
modelmacro is implemented cleanly with good documentation that clearly explains its purpose and functionality. The macro automatically applies useful defaults like#[serde(rename_all = "camelCase")]and derives appropriate traits for different data types.It was a good choice to delegate the actual implementation to a separate
modelmodule, which helps maintain separation of concerns and makes the codebase more maintainable.warframe-macros/src/model/struct_impl/return_style.rs (1)
9-13: LGTM: Clear enum definition with appropriate derivesThe
ReturnStyleenum is well defined with the necessary derive macros for its purpose.src/worldstate/models/deep_archimedia.rs (2)
5-10: Well-defined structure with appropriate attributesThe
DeepArchimediastruct is well-defined with themodelattribute specifying the endpoint and return style. The use of a fixed-size array for missions reflects the expected structure from the API.
12-18: Good use of type renaming for clearer field namingUsing
#[serde(rename = "mission")]for ther#typefield is a good approach to maintain idiomatic Rust naming while matching the API's JSON structure.src/worldstate/mod.rs (7)
29-30: Changingmodelsfrom public to private is a clean architectural choice.By moving
modelsbehindqueryablere-exports, you maintain a curated public interface while retaining internal flexibility. This is an effective design pattern for reducing coupling.
34-35: Nice separation of test fixtures.Exporting the
fixturesmodule only when tests are enabled keeps production code clean. This approach effectively encapsulates test-only logic.
37-61: Well-structuredqueryablemodule.Re-exporting specific types from
modelsneatly organizes the public API. This helps maintain clarity about which structs and enums are intended for external users.
63-69: Clear aggregator of core components.Re-exporting
Client,Error, andLanguagein one place promotes discoverability for downstream consumers. This file now becomes a definitive entry point to the library’s functionality.
70-110: Comprehensive public exposure of model types.Consolidating these model imports under a single namespace is beneficial for code readability. It’s now obvious which game features and states are accessible.
111-113: Forwardingmodelmacro is a convenient developer experience.Re-exporting the macro fosters consistency and lowers the barrier for consumers who may need to create custom models. Good practice in library design.
3-21:Details
❓ Verification inconclusive
Ensure doc example remains valid.
These doc comments provide a clear usage example. To guarantee that the snippet compiles and runs as intended, consider adding a doc test and verifying it with the Rust documentation tests:
🏁 Script executed:
#!/bin/bash # Attempt to compile and run doc tests for the 'worldstate' module. cargo test --doc --package warframeLength of output: 133
Doc Example Validity – Manual Verification Needed
The documentation example in src/worldstate/mod.rs appears correct on inspection, but our automated attempt to run the doc tests failed because the Rust toolchain isn’t available in the current environment. Please manually verify that the snippet compiles and runs as intended:
- Ensure that the Rust toolchain (with Cargo) is installed.
- From your local environment, run:
cargo test --doc --package warframe- Confirm that the documentation tests pass without error.
warframe-macros/src/model/struct_impl/args.rs (6)
25-28:QueryableParamsstruct design aligns well with macro expansions.Storing
endpointandreturn_stylein a single struct ensures future extensibility. This is a neat approach to grouping parameters for the procedural macros.
30-33:QueryableImpleffectively centralizes macro-generated code.Keeping optional
impl_endpointandimpl_queryableas separateTokenStreams cleanly separates the concerns of constructing trait implementations.
59-70: Returning different collection styles viaReturnStyleis flexible.This approach cleanly handles both single-object and multi-object endpoints. The
Queryabletrait is simple yet powerful. No obvious issues here.
72-82: Graceful composition intry_from_queryable_params.Exception handling with the
?operator ensures clearer error propagation, and the function remains easy to read. Good structure overall.
84-89:Argsstruct neatly organizes macro arguments.This layout confirms the design is forward-looking, handling optional fields in a well-defined manner.
91-186: Robust parsing logic inimpl Parse for Args.
- Excellent usage of
syn’s meta parser to handle nested attributes likeexpiry(...)andactivation(...).- The error messages are helpful, ensuring immediate developer feedback on incorrect usage.
- The check enforcing
timed = trueforexpiryandactivationattributes is well placed to guard against invalid states.src/worldstate/fixtures/mod.rs (2)
1-22: Extensive fixture modules organized for each domain.This structured layout ensures test data is isolated by feature, keeping test logic maintainable and discoverable. Good separation of concerns.
24-41:fixturemacro provides a concise testing approach.By generating both
_enand default fixture functions, you effectively manage different localizations or versions of test data. This logic is straightforward and easy to extend in the future.src/worldstate/models/mod.rs (9)
7-7: Docs reference looks fine.This line references the
Clientstruct for querying, which appears correct and consistent with the rest of the documentation.
15-21: Updated documentation for Queryable usage is well structured.All references to
CetusandFissurealign logically with the introducedErrortype and theQueryabletrait.Also applies to: 24-24, 27-28
34-36: Mod statements are logically placed and consistent.Exporting these modules publicly clarifies the API surface of the library, and the naming appears self-explanatory.
Also applies to: 38-63
65-65: Imports forClient,Error, andLanguageare consistent.The newly introduced object references flow well within the module. No concerns regarding naming or usage here.
Also applies to: 67-70
73-74: NewItemStringWrapperstruct seems well-defined.Deriving
PartialOrdandOrdfor a string wrapper is acceptable if sorting or ordering is needed.
76-95:querymethod appears coherent.This provides a convenient funnel to
client.query_item; no issues in error handling or concurrency are apparent.
97-119:query_using_langmethod is consistent with the multilingual approach.The method logic is straightforward and maintains consistency with
query_item_using_lang.
121-130: Accessors align with typical Rust naming conventions.
innerandinto_innermatch common patterns for wrapper types.
132-135:AsRef<str>implementation is concise and idiomatic.The implementation ensures
ItemStringWrappercan be seamlessly treated as astr.src/worldstate/client.rs (4)
1-2: Allowing missing error docs is acceptable given the specialized usage.This file extensively documents errors inline. Suppressing the lint keeps the codebase clean.
86-91:fetchmethod properly delegates to theQueryabletrait.Error handling via the unified
Errortype is consistent.
118-123:fetch_using_langcomplementsfetchwith language support.This approach is consistent with the library’s localization strategy.
147-153:query_item,query_item_using_lang, andquery_by_urlstraightforwardly handle item lookups.Type-safety and error handling are properly addressed. The use of
DummyCategoryfor intermediate deserialization is clear.Also applies to: 183-185, 193-198
WalkthroughThe changes comprise extensive refactoring of CI/CD workflows, documentation, and core modules. The GitHub Actions workflow, Cargo manifest, and README have been updated to use a new Rust edition, a bumped MSRV (1.85), and simplified examples. In the core code, macro-based model definitions have been replaced by attribute-based ones across both market and worldstate modules. Unused test functions making actual HTTP requests have been removed in favor of fixture-based tests using Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant Cache as Cache
participant Req as reqwest::Client
C->>Cache: Check for cached response (get_from_cache)
alt Cache Hit
Cache-->>C: Return cached data
else Cache Miss
C->>Req: Execute HTTP GET (fetch_using_language)
Req-->>C: Return API response
C->>Cache: Insert data into cache (insert_into_cache)
end
C-->>Caller: Return fetched data
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope or unrelated code changes were found. All changes align with the stated objectives in the linked issues. Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/he/ck/heck, error: Permission denied (os error 13) Caused by: ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/worldstate/client.rs (1)
451-491:⚠️ Potential issuePotential panic on negative durations remains unfixed
Similar to the issue on line 292, the code on line 485 still uses
.to_std().unwrap()without checking for negative durations, which could lead to a panic.Add a safety check before converting the duration:
let time_to_sleep = item.expiry() - chrono::offset::Utc::now(); tracing::debug!( listener = %std::any::type_name::<T>(), sleep_duration = %time_to_sleep.num_seconds(), "(LISTENER) Sleeping" ); - tokio::time::sleep(time_to_sleep.to_std().unwrap()).await; + // Ensure we don't sleep for a negative duration + let duration = time_to_sleep.to_std().unwrap_or_else(|_| std::time::Duration::from_secs(1)); + tokio::time::sleep(duration).await;
♻️ Duplicate comments (3)
src/worldstate/fixtures/global_upgrade.rs (1)
35-36:⚠️ Potential issueFix inconsistent duration values between fixtures.
The
etavalue differs between the English (1d 53m 9s) and Chinese (1d 52m 29s) fixtures. This inconsistency is also reflected in their respective descriptions. The durations should match exactly.Apply this diff to fix the inconsistency:
- "eta": "1d 52m 29s", - "desc": "2x 任務擊殺經驗 for 1d 52m 29s" + "eta": "1d 53m 9s", + "desc": "2x 任務擊殺經驗 for 1d 53m 9s"src/worldstate/fixtures/syndicate_mission.rs (1)
521-521:⚠️ Potential issueFix typo in field name.
There's a typo in the field name
timeBooundwhich should betimeBoundto match the field name used in line 151.Apply this diff to fix the typo:
- "timeBoound": "night" + "timeBound": "night"src/worldstate/client.rs (1)
257-293:⚠️ Potential issuePotential panic on negative durations remains unfixed
The code still uses
.to_std().unwrap()on line 292 without checking for negative durations. Ifitem.expiry()occurs in the past due to clock offsets or data anomalies, this will panic.Add a safety check before converting the duration:
let time_to_sleep = item.expiry() - chrono::offset::Utc::now(); tracing::debug!( listener = %std::any::type_name::<T>(), sleep_duration = %time_to_sleep.num_seconds(), "(LISTENER) Sleeping" ); - tokio::time::sleep(time_to_sleep.to_std().unwrap()).await; + // Ensure we don't sleep for a negative duration + let duration = time_to_sleep.to_std().unwrap_or_else(|_| std::time::Duration::from_secs(1)); + tokio::time::sleep(duration).await;
🧹 Nitpick comments (38)
src/worldstate/fixtures/cetus.rs (1)
1-30: Well-structured test fixture.The fixture provides two variations of the same JSON data with a slight difference in the
timeLeftfield, which is useful for testing time-related functionality. The use of a macro for fixture definition provides a consistent pattern across the codebase.Consider adding a comment explaining the purpose of having two different timeLeft values ("51m 20s" vs "51m 0s") to make the intent clearer for future developers.
src/worldstate/fixtures/arbitration.rs (1)
3-32: Consider differentiating between test fixturesThe two JSON objects defined in this fixture (lines 6-16 and 20-30) are identical. For testing purposes, it's generally more valuable to have fixtures with different values to cover various scenarios.
Also, note that the fixture uses extreme dates:
- The activation date is set to Unix epoch (1970-01-01)
- The expiry date is set to a very far future (+275760-09-13)
While these might be intentional for testing edge cases, consider adding more realistic date values if this fixture will be used for general testing.
src/worldstate/models/items/warframe.rs (1)
12-12: Consider alternatives to multiple boolean fieldsThe addition of
#[allow(clippy::struct_excessive_bools)]suppresses a valid warning about the struct having too many boolean fields. While this is acceptable if the structure is dictated by an external API, consider if some of these booleans could be consolidated into enum types or if they represent state that could be modeled differently.For example, fields like
conclave,consume_on_build,is_prime,masterable, andtradablemight represent categories or states that could be modeled as enums or bitflags if they're related.src/market/models/i18n.rs (1)
12-38: Consider using consistent naming convention for enum variantsThe
Languageenum uses PascalCase for variant names (e.g.,Ko,Ru,ZhHans) while the serde rename attributes use the standard ISO language codes (e.g.,ko,ru,zh-hans). While functionally correct, consider using a more descriptive naming convention for the variants to improve code readability:- Ko, + Korean, - Ru, + Russian, - ZhHans, + ChineseSimplified,This would make the code more self-documenting while maintaining the correct serialization format.
src/worldstate/fixtures/flash_sale.rs (1)
1078-2149: Consider adding more variation between the alternative fixture data sets.While having two data sets is good for testing, they differ primarily in the "eta" values with minimal structural differences. Consider adding more meaningful variations between the two sets (like different discount values, expired items, etc.) to provide better test coverage for different behaviors.
src/worldstate/fixtures/steel_path.rs (1)
90-91: Fix typo in "Trio Orbit Ephermera"There's a spelling error in the item name:
- "name": "Trio Orbit Ephermera", + "name": "Trio Orbit Ephemera",The correct spelling is "Ephemera" (with an 'e'), not "Ephermera".
src/market/models/versions.rs (1)
25-40: Consider formatting consistency in Collections struct.While functionally correct, the
Collectionsstruct has inconsistent spacing between fields, unlike the other structs in this file. Consider making the formatting consistent for better readability.#[derive(Debug, Clone, PartialEq, Deserialize)] pub struct Collections { pub items: String, - pub rivens: String, - pub liches: String, - pub sisters: String, - pub missions: String, - pub npcs: String, - pub locations: String, }src/market/models/fixtures/item.json (1)
24-32: Consider adding more languages for comprehensive i18n testing.The current fixture only includes English internationalization data. For more comprehensive testing of the i18n functionality, consider adding at least one more language, especially if the application supports multiple languages.
Example addition:
"i18n": { "en": { "name": "Mirage Prime Set", "description": "Dazzle the opposition with this golden master of illusion and mayhem. Featuring altered mod polarities allow for greater customization.", "wikiLink": "https://wiki.warframe.com/w/Mirage_Prime", "icon": "items/images/en/mirage_prime_set.e7f8f484dd6ae6c35f0767fff35a5109.png", "thumb": "items/images/en/thumbs/mirage_prime_set.e7f8f484dd6ae6c35f0767fff35a5109.128x128.png" + }, + "fr": { + "name": "Ensemble Mirage Prime", + "description": "Éblouissez l'opposition avec ce maître doré de l'illusion et du chaos. Présente des polarités de mod modifiées pour une plus grande personnalisation.", + "wikiLink": "https://wiki.warframe.com/w/Mirage_Prime", + "icon": "items/images/fr/mirage_prime_set.e7f8f484dd6ae6c35f0767fff35a5109.png", + "thumb": "items/images/fr/thumbs/mirage_prime_set.e7f8f484dd6ae6c35f0767fff35a5109.128x128.png" } }src/worldstate/fixtures/item.rs (1)
27-264: Consider standardizing fixture definition approach.The
nanospores_defixture is comprehensive and provides excellent test data with detailed drop information, parent items, and patch logs. However, I noticed it uses a different definition approach (#[rstest::fixture]) compared to thefixture!macro used foritem_sigil.Consider standardizing the approach to defining fixtures throughout the codebase for consistency, either using the
fixture!macro for all fixtures or the#[rstest::fixture]attribute.src/market/models/item_short.rs (1)
55-70: Robust test implementation using rstestThe test module appropriately uses rstest for fixture-based testing, which is in line with the broader refactoring mentioned in the PR objectives. Consider adding assertions to verify specific fields after deserialization for even more robust testing.
fn test_item_short( #[files("src/market/models/fixtures/items.json")] #[mode = str] json: &str, ) -> Result<(), serde_json::Error> { - serde_json::from_str::<ResponseBase<Vec<ItemShort>>>(json)?; + let items = serde_json::from_str::<ResponseBase<Vec<ItemShort>>>(json)?; + + // Verify items were properly deserialized + assert!(!items.payload.is_empty(), "Expected at least one item to be deserialized"); + + // Optional: Verify specific expected fields for a known item + // if let Some(first_item) = items.payload.first() { + // assert!(!first_item.id.is_empty(), "Item ID should not be empty"); + // } Ok(()) }src/worldstate/models/steel_path.rs (1)
27-52: Simple but effective test implementation for SteelPathThe test module correctly validates that the JSON data can be deserialized into the
SteelPathstructure. Consider adding more specific assertions to validate that the deserialized data has the expected structure and contents.#[rstest] fn test(steel_path_en: &str) { - from_str::<R>(steel_path_en).unwrap(); + let result = from_str::<R>(steel_path_en).unwrap(); + + // Verify specific fields in the deserialized result + assert!(!result.current_offer.name.is_empty(), "Current offer name should not be empty"); + assert!(result.current_offer.cost > 0, "Current offer cost should be positive"); + + // Verify rotation and evergreens collections + assert!(!result.rotation.is_empty(), "Rotation should not be empty"); + assert!(!result.evergreens.is_empty(), "Evergreens should not be empty"); }This enhances the test to verify the structure and basic content expectations.
src/worldstate/models/mission_type.rs (1)
1-108: Improved enum definition with explicit serialization attributesThe transition from macro-based to attribute-based enum definition significantly improves code clarity and maintainability. Each variant is now well-documented and the serialization attributes explicitly show how each variant is represented in JSON.
While the code is well-structured, consider adding a test module similar to other model files to verify correct serialization and deserialization of the enum values, especially for variants with custom rename attributes.
#[cfg(test)] mod tests { use super::MissionType; use serde::{Deserialize, Serialize}; use serde_json::{from_str, to_string}; #[test] fn test_mission_type_serialization() { // Test a few representative variants let variants = vec![ MissionType::DarkSectorDefense, MissionType::Extermination, MissionType::VoidCascade, ]; for variant in variants { let serialized = to_string(&variant).unwrap(); let deserialized: MissionType = from_str(&serialized).unwrap(); assert_eq!(variant, deserialized, "Serialization roundtrip failed"); } } }warframe-macros/src/model/enum_impl.rs (1)
42-63: Improve robustness of Opposite trait implementationThe current implementation uses separate
unwrap()calls onfirst()andlast(). While safe in this context (since you've verified there are exactly 2 variants), a pattern match would be more robust.let opposite_trait_impl = match item.variants.len() { 2 => { - let (a, b) = ( - &item.variants.first().unwrap().ident, - &item.variants.last().unwrap().ident, - ); + let [first, second] = &item.variants[..] else { + unreachable!("We've already checked there are exactly 2 variants"); + }; + let (a, b) = (&first.ident, &second.ident); let type_name = &item.ident; Some(quote! { impl crate::worldstate::models::base::Opposite for #type_name { fn opposite(&self) -> Self { match self { #type_name::#a => #type_name::#b, #type_name::#b => #type_name::#a, } } } }) } _ => None, };devtools.nu (1)
18-28: Document the difference in language handling between APIsThe
market_reqfunction uses headers for language whileworldstate_requses query parameters. Consider adding a comment to clarify this difference.-# Returns the response of a GET request to the specified endpoint +# Returns the response of a GET request to the specified endpoint +# Note: market API uses headers for language specification export def market_req [src/worldstate/models/deep_archimedea.rs (1)
27-65: Consider moving commented enum to a separate file or issueThe commented-out
Deviationenum is quite large and takes up significant space. Consider either:
- Moving it to a separate file if it's planned for future implementation
- Creating an issue to track its implementation later
- Adding more context about why it's not currently implemented and when it might be needed
This would improve code readability while maintaining the valuable information.
src/worldstate/models/news.rs (2)
58-59: Consider usingexpect(...)instead ofunwrap().
While this is a test,expect(...)helps produce clearer error messages.
63-64: Suggest more descriptive error handling.
Similarly,expect("Deserialization failed")would provide a clearer error message.src/worldstate/models/faction.rs (2)
40-80:vulnerable_tofunction is clear but might benefit from a data-driven approach.
Currently, maintaining a repetitive match block works, but if more factions and damage types are added, a mapping or table-driven approach may reduce repetition.
85-103:resistant_tofunction is logically consistent.
Likevulnerable_to, a data-driven approach could streamline maintenance if new factions are introduced.src/worldstate/models/flash_sale.rs (2)
44-45: In tests, preferexpectfor clearer failures.
unwrap()is acceptable in a test, but a descriptive message aids debugging.
49-50: Consider adding a descriptive error message.
unwrap()might hide important details in case of deserialization failure.warframe-macros/src/model/struct_impl/mod.rs (1)
29-54:append_activation_and_expiryfunction logic is fine but consider conflicts.
Appending theactivationandexpiryfields works if those fields don’t already exist. A guard against duplicate field names might improve robustness.src/worldstate/models/construction_progress.rs (1)
12-18: Clarify theunknown_progressfield
While the code mentions "No clue what this is tbh," consider adding a TODO note or further context to make maintenance easier.src/worldstate/models/fissure.rs (1)
66-91: Tests correctly verify JSON deserialization.
Usingrstestis a concise approach for parameterized tests. One recommendation: consider adding negative tests (e.g., invalid JSON or missing fields) to confirm robust error handling. If this is out of scope, then these tests are fine for ensuring the basic success path.src/worldstate/models/invasion.rs (4)
1-2: Consider grouping related imports together.
Grouping imports by domain (e.g., local modules vs. external crates) can improve readability.To maintain clarity, consider arranging them like:
-use warframe_macros::model; -use super::{ +use super::{ faction::Faction, reward::Reward, reward_type::RewardType, }; +use warframe_macros::model;Also applies to: 4-6
11-13: Adopt consistent article usage in doc comments.
“An defender/attacker” should be “A defender/attacker.”-/// An defender/attacker of an Invasion +/// A defender/attacker of an Invasion
17-18: Validate the data consistency forfactionvsfaction_key.
You are storing both a string representation (faction: String) and a typed key (faction_key: Faction). If you only ever handle recognized key-based factions, consider whether duplicating this data is necessary or if only one is sufficient.Also applies to: 20-21
72-82: Parameterize tests for better coverage.
While the#[rstest]approach is good, consider adding assertions to confirm non-empty or expected values in the resulting deserialized objects, ensuring you catch potential future data format changes.Example:
fn test(invasion_en: &str) { let result = from_str::<R>(invasion_en).unwrap(); + // For example, check for some known fields to be properly deserialized + assert!(!result.is_empty(), "Expected result not to be empty"); }src/market/mod.rs (3)
2-3: Expand the module-level documentation further.
You have a short mention, but consider including a summary of the module’s features and usage principles, helping new contributors understand the context quickly.
36-36: BASE_URL usage.
The hardcoded base URL is straightforward, but consider if it needs environment-based overrides or configuration for local testing or future changes.
44-49: Consider additional transformations.
If your input can contain special or non-ASCII characters, you may want to sanitize or transliterate them to ensure consistent slugs.src/market/models/mod.rs (2)
7-16: Keep consistent import style.
Imports fromstd::fmt::Debug,serde(with submodules), andi18n::Language, plus referencingBASE_URLcan be grouped for clarity.
31-42: Asynchronous query method looks correct.
Consider whether you need timeouts, error retry logic, or more robust error handling if the remote endpoint is less reliable.src/worldstate/models/void_trader.rs (1)
47-55: Consider adding edge case testsWhile the basic deserialization tests are good, consider adding tests for edge cases such as empty inventory arrays or unusual item names to ensure robust handling of all possible API responses.
#[rstest] fn test_empty_inventory() { let json = r#"{"id":"test","location":"Test Location","inventory":[]}"#; let result = from_str::<R>(json).unwrap(); assert!(result.inventory.is_empty()); }src/worldstate/fixtures/mod.rs (1)
24-41: Macro usage is correct and well-structured.This fixture macro provides a clean way to create two test fixtures under different names. One minor suggestion is to accept more general token trees (
$body2:tt) if you ever need multi-line expansions, but for now, using$body2:literalis acceptable for basic string fixtures.src/market/client.rs (1)
45-86: Consider making the cache cap configurable in theClientbuilder.Having a fixed max capacity (1000) for the cache fields might be limiting if the usage scales. Consider exposing these limits as builder fields to allow custom configuration.
src/worldstate/client.rs (1)
1-2: Consider using specific allow attributes instead of broad onesThe
#![allow(clippy::missing_errors_doc)]directive suppresses all missing error documentation warnings in the file. It would be more maintainable to add this directive only to specific functions where documenting each possible error is impractical.src/worldstate/models/mod.rs (1)
7-8: Typo in documentationThere appears to be a formatting error in the documentation comment. The sentence about querying through the
Clientis incomplete or incorrectly formatted.- //! [`Client`](crate::worldstate::client::Client). # Querying... + //! [`Client`](crate::worldstate::client::Client) is used for querying. # Querying...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (108)
.github/workflows/release.yml(1 hunks).vscode/test.code-snippets(1 hunks)CONTRIBUTING.md(0 hunks)Cargo.toml(2 hunks)LICENSE(1 hunks)README.md(1 hunks)devtools.nu(1 hunks)rustfmt.toml(1 hunks)src/lib.rs(0 hunks)src/market/cache.rs(1 hunks)src/market/client.rs(1 hunks)src/market/error.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/fixtures/item.json(1 hunks)src/market/models/fixtures/versions.json(1 hunks)src/market/models/i18n.rs(1 hunks)src/market/models/item.rs(1 hunks)src/market/models/item_info.rs(0 hunks)src/market/models/item_short.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/orders.rs(0 hunks)src/market/models/set_items.rs(1 hunks)src/market/models/statistic_item.rs(0 hunks)src/market/models/versions.rs(1 hunks)src/worldstate/client.rs(14 hunks)src/worldstate/error.rs(1 hunks)src/worldstate/fixtures/alert.rs(1 hunks)src/worldstate/fixtures/arbitration.rs(1 hunks)src/worldstate/fixtures/archon_hunt.rs(1 hunks)src/worldstate/fixtures/cambion_drift.rs(1 hunks)src/worldstate/fixtures/cetus.rs(1 hunks)src/worldstate/fixtures/construction_progress.rs(1 hunks)src/worldstate/fixtures/daily_deal.rs(1 hunks)src/worldstate/fixtures/deep_archimedea.rs(1 hunks)src/worldstate/fixtures/event.rs(1 hunks)src/worldstate/fixtures/fissure.rs(1 hunks)src/worldstate/fixtures/flash_sale.rs(1 hunks)src/worldstate/fixtures/global_upgrade.rs(1 hunks)src/worldstate/fixtures/invasion.rs(1 hunks)src/worldstate/fixtures/item.rs(1 hunks)src/worldstate/fixtures/mod.rs(1 hunks)src/worldstate/fixtures/news.rs(1 hunks)src/worldstate/fixtures/nightwave.rs(1 hunks)src/worldstate/fixtures/orb_vallis.rs(1 hunks)src/worldstate/fixtures/sortie.rs(1 hunks)src/worldstate/fixtures/steel_path.rs(1 hunks)src/worldstate/fixtures/syndicate_mission.rs(1 hunks)src/worldstate/fixtures/void_trader.rs(1 hunks)src/worldstate/fixtures/weapon.rs(1 hunks)src/worldstate/language.rs(2 hunks)src/worldstate/listener.rs(0 hunks)src/worldstate/mod.rs(1 hunks)src/worldstate/models/alert.rs(1 hunks)src/worldstate/models/arbitration.rs(1 hunks)src/worldstate/models/archon_hunt.rs(1 hunks)src/worldstate/models/base.rs(6 hunks)src/worldstate/models/cambion_drift.rs(1 hunks)src/worldstate/models/cetus.rs(1 hunks)src/worldstate/models/construction_progress.rs(1 hunks)src/worldstate/models/daily_deal.rs(1 hunks)src/worldstate/models/damage_type.rs(3 hunks)src/worldstate/models/deep_archimedea.rs(1 hunks)src/worldstate/models/event.rs(1 hunks)src/worldstate/models/faction.rs(3 hunks)src/worldstate/models/fissure.rs(1 hunks)src/worldstate/models/flash_sale.rs(1 hunks)src/worldstate/models/global_upgrades.rs(1 hunks)src/worldstate/models/invasion.rs(1 hunks)src/worldstate/models/items/arcane.rs(0 hunks)src/worldstate/models/items/archwing.rs(1 hunks)src/worldstate/models/items/fish.rs(0 hunks)src/worldstate/models/items/gear.rs(0 hunks)src/worldstate/models/items/glyph.rs(0 hunks)src/worldstate/models/items/misc.rs(0 hunks)src/worldstate/models/items/mod.rs(4 hunks)src/worldstate/models/items/modification.rs(1 hunks)src/worldstate/models/items/node.rs(0 hunks)src/worldstate/models/items/pet.rs(0 hunks)src/worldstate/models/items/quest.rs(0 hunks)src/worldstate/models/items/relic.rs(0 hunks)src/worldstate/models/items/resource.rs(0 hunks)src/worldstate/models/items/sentinel.rs(1 hunks)src/worldstate/models/items/sigil.rs(0 hunks)src/worldstate/models/items/skin.rs(0 hunks)src/worldstate/models/items/warframe.rs(1 hunks)src/worldstate/models/items/weapon.rs(6 hunks)src/worldstate/models/macros.rs(0 hunks)src/worldstate/models/mission.rs(1 hunks)src/worldstate/models/mission_type.rs(1 hunks)src/worldstate/models/mod.rs(1 hunks)src/worldstate/models/news.rs(1 hunks)src/worldstate/models/nightwave.rs(2 hunks)src/worldstate/models/orb_vallis.rs(1 hunks)src/worldstate/models/reward.rs(1 hunks)src/worldstate/models/reward_type.rs(1 hunks)src/worldstate/models/sortie.rs(1 hunks)src/worldstate/models/steel_path.rs(1 hunks)src/worldstate/models/syndicate.rs(1 hunks)src/worldstate/models/syndicate_mission.rs(1 hunks)src/worldstate/models/void_trader.rs(1 hunks)src/worldstate/utils.rs(1 hunks)warframe-macros/Cargo.toml(1 hunks)warframe-macros/src/lib.rs(1 hunks)warframe-macros/src/model/enum_impl.rs(1 hunks)warframe-macros/src/model/mod.rs(1 hunks)warframe-macros/src/model/struct_impl/args.rs(1 hunks)warframe-macros/src/model/struct_impl/mod.rs(1 hunks)warframe-macros/src/model/struct_impl/return_style.rs(1 hunks)
💤 Files with no reviewable changes (19)
- src/worldstate/models/items/glyph.rs
- src/worldstate/models/items/gear.rs
- src/worldstate/models/items/pet.rs
- src/worldstate/models/items/sigil.rs
- src/worldstate/models/items/node.rs
- src/worldstate/models/items/skin.rs
- src/worldstate/models/items/fish.rs
- src/lib.rs
- src/worldstate/models/items/resource.rs
- src/worldstate/models/items/misc.rs
- src/worldstate/models/items/quest.rs
- src/worldstate/models/items/relic.rs
- CONTRIBUTING.md
- src/worldstate/models/items/arcane.rs
- src/market/models/item_info.rs
- src/market/models/orders.rs
- src/worldstate/models/macros.rs
- src/market/models/statistic_item.rs
- src/worldstate/listener.rs
🧰 Additional context used
🧬 Code Definitions (34)
warframe-macros/src/model/struct_impl/return_style.rs (2)
warframe-macros/src/model/mod.rs (1)
syn(13-13)warframe-macros/src/model/struct_impl/args.rs (1)
parse(92-185)
src/worldstate/models/news.rs (17)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/daily_deal.rs (3)
test(47-49)from_str(48-48)from_str(53-53)src/worldstate/models/event.rs (3)
test(72-74)from_str(73-73)from_str(78-78)src/worldstate/models/flash_sale.rs (3)
test(44-46)from_str(45-45)from_str(50-50)src/worldstate/models/fissure.rs (3)
test(82-84)from_str(83-83)from_str(88-88)src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/syndicate_mission.rs (3)
test(69-71)from_str(70-70)from_str(75-75)src/worldstate/models/alert.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/arbitration.rs (3)
test(99-101)from_str(100-100)from_str(105-105)src/worldstate/models/global_upgrades.rs (1)
test(39-41)src/worldstate/models/cambion_drift.rs (1)
test(40-42)src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/construction_progress.rs (1)
test(38-40)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/void_trader.rs (1)
test(48-50)
src/worldstate/models/invasion.rs (3)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/sortie.rs (2)
test(56-58)test_ml(61-63)
src/worldstate/models/reward.rs (1)
warframe-macros/src/lib.rs (1)
model(21-23)
warframe-macros/src/model/struct_impl/mod.rs (3)
warframe-macros/src/model/mod.rs (1)
syn(13-13)src/worldstate/models/base.rs (2)
activation(35-35)expiry(38-38)warframe-macros/src/model/struct_impl/args.rs (1)
try_from_queryable_params(73-81)
src/worldstate/models/orb_vallis.rs (16)
src/worldstate/models/cambion_drift.rs (3)
test(40-42)from_str(41-41)from_str(46-46)src/worldstate/models/construction_progress.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/flash_sale.rs (3)
test(44-46)from_str(45-45)from_str(50-50)src/worldstate/models/fissure.rs (3)
test(82-84)from_str(83-83)from_str(88-88)src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/nightwave.rs (3)
test(106-108)from_str(107-107)from_str(112-112)src/worldstate/models/arbitration.rs (3)
test(99-101)from_str(100-100)from_str(105-105)src/worldstate/models/archon_hunt.rs (3)
test(78-80)from_str(79-79)from_str(84-84)src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/daily_deal.rs (1)
test(47-49)src/worldstate/models/event.rs (1)
test(72-74)src/worldstate/models/news.rs (1)
test(58-60)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/syndicate_mission.rs (1)
test(69-71)src/worldstate/models/void_trader.rs (1)
test(48-50)src/worldstate/models/alert.rs (1)
test(38-40)
src/worldstate/models/global_upgrades.rs (13)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/daily_deal.rs (3)
test(47-49)from_str(48-48)from_str(53-53)src/worldstate/models/event.rs (3)
test(72-74)from_str(73-73)from_str(78-78)src/worldstate/models/flash_sale.rs (3)
test(44-46)from_str(45-45)from_str(50-50)src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/syndicate_mission.rs (3)
test(69-71)from_str(70-70)from_str(75-75)src/worldstate/models/alert.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/construction_progress.rs (1)
test(38-40)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/void_trader.rs (1)
test(48-50)
src/worldstate/models/alert.rs (4)
src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/news.rs (1)
test(58-60)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/sortie.rs (1)
test(56-58)
src/worldstate/models/mission.rs (1)
warframe-macros/src/lib.rs (1)
model(21-23)
src/worldstate/models/mission_type.rs (1)
warframe-macros/src/lib.rs (1)
model(21-23)
src/worldstate/models/steel_path.rs (5)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/news.rs (2)
test(58-60)test_ml(63-65)src/worldstate/models/sortie.rs (2)
test(56-58)test_ml(61-63)src/worldstate/models/alert.rs (2)
test(38-40)test_ml(43-45)
src/worldstate/models/syndicate_mission.rs (7)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/event.rs (3)
test(72-74)from_str(73-73)from_str(78-78)src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/void_trader.rs (1)
test(48-50)
src/worldstate/models/fissure.rs (3)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/sortie.rs (2)
test(56-58)test_ml(61-63)
warframe-macros/src/lib.rs (1)
warframe-macros/src/model/mod.rs (2)
syn(13-13)expand(12-21)
src/market/models/item_short.rs (2)
src/market/models/item.rs (1)
serde_json(63-63)src/market/models/versions.rs (1)
serde_json(53-53)
src/market/mod.rs (3)
src/market/models/mod.rs (1)
client(36-41)src/market/client.rs (1)
new(97-99)src/market/cache.rs (1)
new(16-21)
src/worldstate/models/reward_type.rs (1)
warframe-macros/src/lib.rs (1)
model(21-23)
warframe-macros/src/model/mod.rs (2)
warframe-macros/src/model/enum_impl.rs (1)
parse_enum(13-72)warframe-macros/src/model/struct_impl/mod.rs (1)
parse_struct(56-118)
src/worldstate/models/flash_sale.rs (6)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/news.rs (4)
test(58-60)from_str(59-59)from_str(64-64)test_ml(63-65)src/worldstate/models/alert.rs (2)
test(38-40)test_ml(43-45)src/worldstate/models/nightwave.rs (2)
test(106-108)test_ml(111-113)src/worldstate/models/cetus.rs (2)
test(39-41)test_ml(44-46)
src/worldstate/models/cetus.rs (3)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/news.rs (2)
test(58-60)test_ml(63-65)
src/worldstate/models/void_trader.rs (5)
src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/news.rs (2)
test(58-60)test_ml(63-65)src/worldstate/models/nightwave.rs (2)
test(106-108)test_ml(111-113)src/worldstate/models/sortie.rs (2)
test(56-58)test_ml(61-63)src/worldstate/models/alert.rs (2)
test(38-40)test_ml(43-45)
src/worldstate/models/construction_progress.rs (1)
src/worldstate/models/base.rs (1)
deserialize_f32_from_string(114-120)
src/worldstate/models/event.rs (8)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/alert.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/void_trader.rs (1)
test(48-50)
src/worldstate/models/syndicate.rs (1)
warframe-macros/src/lib.rs (1)
model(21-23)
src/worldstate/models/archon_hunt.rs (2)
src/worldstate/models/alert.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/news.rs (1)
test(58-60)
src/worldstate/models/daily_deal.rs (2)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)
src/worldstate/models/cambion_drift.rs (12)
src/worldstate/models/construction_progress.rs (3)
test(38-40)from_str(39-39)from_str(44-44)src/worldstate/models/flash_sale.rs (3)
test(44-46)from_str(45-45)from_str(50-50)src/worldstate/models/invasion.rs (3)
test(88-90)from_str(89-89)from_str(94-94)src/worldstate/models/nightwave.rs (3)
test(106-108)from_str(107-107)from_str(112-112)src/worldstate/models/orb_vallis.rs (3)
test(40-42)from_str(41-41)from_str(46-46)src/worldstate/models/arbitration.rs (3)
test(99-101)from_str(100-100)from_str(105-105)src/worldstate/models/daily_deal.rs (1)
test(47-49)src/worldstate/models/event.rs (1)
test(72-74)src/worldstate/models/news.rs (1)
test(58-60)src/worldstate/models/sortie.rs (1)
test(56-58)src/worldstate/models/syndicate_mission.rs (1)
test(69-71)src/worldstate/models/alert.rs (1)
test(38-40)
src/market/client.rs (3)
src/market/cache.rs (1)
new(16-21)src/market/models/mod.rs (1)
query(31-50)src/worldstate/models/mod.rs (1)
query(93-95)
src/worldstate/models/sortie.rs (3)
warframe-macros/src/lib.rs (1)
model(21-23)src/worldstate/models/base.rs (1)
endpoint(29-29)src/worldstate/client.rs (1)
serde_json(207-207)
src/worldstate/mod.rs (15)
src/worldstate/models/cetus.rs (1)
test(39-41)src/worldstate/models/alert.rs (1)
test(38-40)src/worldstate/models/fissure.rs (1)
test(82-84)src/worldstate/models/arbitration.rs (1)
test(99-101)src/worldstate/models/deep_archimedea.rs (1)
test(84-86)src/worldstate/models/steel_path.rs (1)
test(44-46)src/worldstate/models/archon_hunt.rs (1)
test(78-80)src/worldstate/models/invasion.rs (1)
test(88-90)src/worldstate/models/cambion_drift.rs (1)
test(40-42)src/worldstate/models/construction_progress.rs (1)
test(38-40)src/worldstate/models/flash_sale.rs (1)
test(44-46)src/worldstate/models/nightwave.rs (1)
test(106-108)src/worldstate/models/orb_vallis.rs (1)
test(40-42)src/worldstate/models/daily_deal.rs (1)
test(47-49)src/worldstate/models/event.rs (1)
test(72-74)
src/worldstate/models/base.rs (2)
src/worldstate/models/mod.rs (1)
query(93-95)src/market/models/mod.rs (1)
query(31-50)
src/worldstate/client.rs (6)
src/market/client.rs (7)
items(263-289)items(300-304)new(97-99)fetch(146-151)response(246-246)response(274-274)item(115-115)src/worldstate/models/items/mod.rs (1)
map_category_to_item(218-248)src/worldstate/utils.rs (1)
new(21-23)src/worldstate/models/mod.rs (1)
query(93-95)src/worldstate/models/base.rs (1)
query(76-87)src/market/models/mod.rs (1)
query(31-50)
src/worldstate/models/items/mod.rs (2)
src/market/client.rs (3)
item(115-115)items(263-289)items(300-304)src/worldstate/fixtures/item.rs (1)
nanospores_de(28-264)
src/worldstate/models/mod.rs (3)
src/market/client.rs (2)
items(263-289)items(300-304)src/market/models/mod.rs (2)
client(36-41)query(31-50)src/worldstate/models/base.rs (1)
query(76-87)
🔇 Additional comments (275)
LICENSE (1)
3-3: Copyright holder properly specifiedThe copyright holder has been updated from the placeholder "[fullname]" to the actual name "Mettwasser", which correctly identifies the copyright owner.
rustfmt.toml (1)
16-18: Good formatting configuration additionsEnabling
format_macro_bodiesandformat_macro_matcherswill improve code consistency by ensuring that macros follow the same formatting rules as the rest of the codebase. The commented-out option serves as documentation for potential future configuration.warframe-macros/Cargo.toml (3)
13-14: Correctly configured as a procedural macroThe crate is properly set up with
proc-macro = true, which is required for procedural macro crates.
16-21: Appropriate macro development dependenciesThe dependencies (heck, manyhow, proc-macro2, quote, syn) are well-chosen for procedural macro development. Using the "full" feature for syn ensures access to all parsing capabilities needed for attribute macros.
4-4:Details
✅ Verification successful
Verify Rust edition validity
The Rust edition is set to "2024", but as of my knowledge cutoff, the only valid Rust editions are 2015, 2018, and 2021. This may cause build failures.
🌐 Web query:
Is Rust edition 2024 valid or announced?💡 Result:
The Rust 2024 Edition has been officially announced and is now stable as part of Rust 1.85.0, released in February 2025[1][3][6]. This edition introduces significant updates to the language, tooling, and ecosystem while maintaining backward compatibility through Rust's opt-in edition system[3][8].
Key features of the Rust 2024 Edition include:
- Async closures (
async || {}) for native asynchronous programming with value capture[3][6]- Enhanced safety rules:
externblocks now requireunsafekeyword[3]- Certain functions like
std::env::set_varare markedunsafe[1][3]- Language improvements:
- Revised temporary variable scoping in
if letand match expressions[3]- Extended tuple support (up to 12 items)[1]
- New reserved keywords for future features[1]
- Tooling upgrades:
- Rustfmt "style editions" for independent formatting configuration[1][3]
- Cargo's Rust-version aware dependency resolver[3][4]
Developers can migrate existing projects using
cargo fix --edition, though large codebases may benefit from incremental updates[3][4]. The edition maintains compatibility with prior Rust versions, allowing mixed dependencies across editions[8].While some community discussion occurred about renaming it to 2025 Edition due to the February 2025 release date[2], the official release retains the 2024 designation[3]. This follows Rust's established practice of naming editions by their target year rather than strict release timing[2][8].
Citations:
- 1: https://www.developer-tech.com/news/rust-1-85-0-released-2024-edition-stabilised/
- 2: https://internals.rust-lang.org/t/rename-edition-2024-to-edition-2025/21266
- 3: https://blog.rust-lang.org/2025/02/20/Rust-1.85.0.html
- 4: https://codeandbitters.com/rust-2024-upgrade/
- 5: https://v5.chriskrycho.com/elsewhere/trpl/
- 6: https://www.youtube.com/watch?v=P4-jSupvBFs
- 7: https://blog.jetbrains.com/rust/2024/09/20/how-to-learn-rust/
- 8: https://users.rust-lang.org/t/what-is-the-difference-between-edition-and-version-in-rust/106695
- 9: https://blog.stackademic.com/recommended-learning-resources-for-rust-in-2024-174cb686acc2
- 10: https://en.wikipedia.org/wiki/Rust_(programming_language)
Rust Edition Update Confirmed
The Rust 2024 edition is now valid as of Rust 1.85.0 (released February 2025), so the Cargo.toml entry is correct and does not require any changes.
.vscode/test.code-snippets (2)
2-35: Improved test structure using rstestThe updated test snippet implements a more maintainable approach using
rstestand fixture-based testing. This is a significant improvement that:
- Simplifies test maintenance by separating test data from logic
- Improves test readability with the declarative rstest approach
- Properly handles multilingual testing with separate test functions
The use of
Queryable::Returntype alias is also a nice touch for cleaner code.
36-53: Well-structured fixture macroThe "Fixture" snippet provides a clean, reusable way to define test fixtures, which will help maintain consistency across tests. The syntax aligns well with the testing approach established in the model test snippet.
src/worldstate/models/global_upgrades.rs (2)
1-1: Good use of procedural macros for model definitions.Using the
warframe_macros::modelimport aligns with the PR's objective of refactoring the codebase to use attribute-based model definitions instead of macro-based ones. This improves maintainability and IDE support.
23-46: Test refactoring improves maintainability.Excellent improvements to the test module:
- Renaming to
test_global_upgradeimproves clarity- Using
rstestsimplifies test case definition- Separating multilingual testing from general testing is a good practice
The fixture-based approach is more maintainable than making actual API calls during tests.
Cargo.toml (7)
4-4: Verify Rust edition.The edition is set to "2024" which is not yet released. This should be "2021" until the 2024 edition is officially released.
11-11: Reasonable MSRV specification.Setting
rust-version = "1.85"is a good practice to explicitly document the Minimum Supported Rust Version (MSRV). This helps users understand compatibility requirements.
30-40: Dependencies updated to recent versions.Good job updating to newer versions of dependencies like
thiserror(2.0.11) andderive_more(2.0.1). The addition of new dependencies liketracing,paste, andderive_buildershould help with code maintainability and observability.
42-44: Good testing infrastructure improvements.Adding
rstestandtracing-subscriberas dev-dependencies is a positive change that aligns with the test refactoring seen in the model files. This will make tests more maintainable and easier to debug.
46-47: Excellent addition of linting configuration.Setting Clippy's pedantic level to "warn" will help maintain high code quality standards. This is especially important for a public library.
49-51: Well-structured workspace configuration.Using a workspace with a modern resolver version (3) is a good practice. Including both the main package and the macros package as workspace members will simplify dependency management.
15-20:Details
❓ Verification inconclusive
Default features need verification.
The default features have changed from including "worldstate" to "market_ratelimit" and "market_cache". This is a breaking change for users who rely on the default feature set.
Is this change intentional and documented in release notes or the README? Users upgrading from previous versions might face unexpected behavior.
🏁 Script executed:
#!/bin/bash # Check if worldstate is still a feature or has been folded into the base package rg -A3 "worldstate" Cargo.toml # Check README for documentation about feature changes rg -i "feature" README.mdLength of output: 56
Action Required: Confirm and Document Default Feature Changes
The default features in
Cargo.tomlhave been updated from including"worldstate"to using["market_ratelimit", "market_cache"]. Automated searches inCargo.tomland theREADME.mddid not reveal any documentation or mention of this change. Because this update can be a breaking change for users who previously relied on"worldstate", please verify manually:
- Confirm whether the exclusion of
"worldstate"and the inclusion of"market_ratelimit"and"market_cache"is intentional.- If intentional, update the release notes, README, or a changelog (e.g.,
CHANGELOG.md) to clearly communicate this breaking change to users.src/market/models/set_items.rs (1)
5-10: The SetItems struct lacks public field accessors.The struct fields are private but there are no getter methods provided, which means consumers of this struct won't be able to access the ID or items.
Consider either making the fields public or adding getter methods:
#[derive(Debug, Deserialize, PartialEq, Clone)] pub struct SetItems { - id: String, + pub id: String, - items: Vec<Item>, + pub items: Vec<Item>, }Or alternatively:
impl SetItems { pub fn id(&self) -> &str { &self.id } pub fn items(&self) -> &[Item] { &self.items } }src/worldstate/error.rs (2)
17-17: Approve: Good renaming from ApiError to ErrorRenaming the enum from
ApiErrortoErrorfollows Rust's naming conventions where error types in a module are typically namedError. This simplifies imports and makes the code more idiomatic.
22-22: Documentation improvement with proper code formattingAdding backticks around
serde_jsonimproves the documentation by properly formatting the code reference.src/worldstate/fixtures/cambion_drift.rs (1)
1-26: Approve: Well-structured test fixture for cambion_driftThis test fixture is well-implemented using the
fixture!macro, providing two snapshots of the cambion drift state with different timestamps. The time progression from "59m 10s" to "57m 10s" is valuable for testing time-based state changes.src/worldstate/fixtures/construction_progress.rs (1)
1-22: Approve: Well-structured test fixture for construction progressThis test fixture provides two snapshots of construction progress data with different IDs while maintaining the same progress values. This approach is useful for testing ID changes without progress alterations.
src/worldstate/fixtures/orb_vallis.rs (1)
1-28: Approve: Effective time-progression modeling in orb_vallis fixtureThis fixture excellently models the passage of time with both the
timeLeftfield changing from "15m 7s" to "13m 37s" and the corresponding update toshortString. This approach enables thorough testing of time-sensitive features and countdown displays.src/worldstate/fixtures/daily_deal.rs (2)
17-18: Clarify the purpose of different ETA valuesThe fixture contains different ETA values between the English ("1d 1h 6m 19s") and Chinese ("1d 1h 6m 29s") versions. Is this intentional? If testing internationalization is the primary goal, keeping time values consistent would make the language difference clearer.
Also applies to: 35-36
3-40: Good approach for internationalization testingThe fixture effectively tests how the application handles different language representations for the same item, showing both Latin (Akbronco) and non-Latin (野马双枪) character sets. This is valuable for ensuring the application correctly processes and displays internationalized content.
src/market/models/i18n.rs (2)
1-40: Good internationalization implementationThe design of the
Languageenum andI18N<T>type alias provides a clean and type-safe way to handle internationalized content. The comprehensive trait implementations and proper serde annotations show thoughtful API design.Setting English as the default language is a reasonable choice, and the HashMap-based approach for translations is efficient and flexible.
12-38:Details
❓ Verification inconclusive
Verify language coverage completeness
The enum includes many common languages but verify this covers all the languages your application needs to support. Some potentially missing languages that might be relevant depending on your user base:
- Japanese (
ja)- Dutch (
nl)- Swedish (
sv)- Arabic (
ar)- Hindi (
hi)
🏁 Script executed:
#!/bin/bash # Check if there are any localization strings in the codebase for languages not covered in the enum echo "Searching for potential language codes in the codebase..." rg -i '"[a-z]{2}(-[a-z]{2,4})?"' --type=json --type=rust | grep -v -E '(ko|ru|de|fr|pt|zh-hans|zh-hant|es|it|pl|uk|en)'Length of output: 368
Action: Verify Language Coverage Completeness
The enum in
src/market/models/i18n.rsdefines these languages: ko, ru, de, fr, pt, zh-hans, zh-hant, es, it, pl, uk, and en. However, our codebase search revealed two instances insrc/worldstate/language.rswhere"zh"(viaLanguage::ZH) is used. This raises two points for review:
Chinese Variants Consistency: Confirm that the handling of Chinese locales is intentional. If
"zh"(simplified or ambiguous) is required elsewhere, should it be aligned with either the existingZhHans/ZhHantvariants or should a separate variant be added?Potential Missing Languages: As originally noted, verify if additional languages such as Japanese (
ja), Dutch (nl), Swedish (sv), Arabic (ar), or Hindi (hi) are needed based on your user base.Please review these points to ensure complete and consistent language support across the application.
warframe-macros/src/model/mod.rs (3)
1-7: Well-organized importsThe imports are properly organized and include only what's necessary for the file functionality. Good separation between external crates and local modules.
9-11: Clean module organizationGood separation of concerns by splitting enum and struct implementation logic into separate modules.
12-21: Well-implemented procedural macro entry pointThe
expandfunction provides a clean interface for the procedural macro. It correctly handles different item types and provides meaningful error messages. The pattern matching approach is idiomatic Rust.The function properly delegates to specialized handlers for enums and structs, which helps maintain separation of concerns and keeps the code modular.
src/market/models/fixtures/versions.json (1)
1-23: Appropriate test fixture with future datesThis JSON fixture is well-structured and contains appropriate test data. The use of future dates (2025) clearly indicates this is test data rather than production data.
Note that the base64-encoded strings in the
collectionsobject appear to be encoded timestamps, which is a reasonable approach for testing version comparisons.src/market/cache.rs (3)
1-4: Minimal and focused importsGood practice to import only what's needed. The Arc import is appropriate for the shared ownership pattern used in the CacheKey.
5-13: Well-designed cache key structureThe
CacheKeystruct is well-designed with appropriate trait derivations for a cache key (Hash, PartialEq, Eq, etc.). UsingArc<str>for the endpoint field is an efficient choice that allows for shared ownership without unnecessary cloning.The documentation clearly explains the purpose of the endpoint field.
15-22: Clean and simple constructor implementationThe
newmethod is straightforward and does exactly what's needed - creating a new CacheKey with the provided language and converting the endpoint string to an Arc.src/worldstate/models/items/modification.rs (1)
12-12: Appropriate use of clippy lint suppressionThe
#[allow(clippy::struct_excessive_bools)]attribute is justified here as theModstruct has multiple boolean fields (is_exilus, is_prime, is_utility, tradable, transmutable).This is a reasonable approach when the boolean fields represent distinct concepts and refactoring to a different structure would be impractical or reduce code clarity.
src/worldstate/models/items/archwing.rs (1)
11-11: Appropriate use of clippy lint suppression.The
#[allow(clippy::struct_excessive_bools)]attribute is justified here since theArchwingstruct contains multiple boolean fields (consume_on_build,is_prime,masterable,tradable) that mirror the external API's data structure.README.md (2)
10-11: Well-documented MSRV information.Explicitly stating the Minimum Supported Rust Version helps users and contributors understand the required environment.
14-29: Improved example with better error handling and explicit imports.The changes to the example code are positive:
- Moving from prelude to explicit imports improves clarity about what's being used
- Changing from
ApiErrortoErroraligns with the codebase's error handling updates- Using the
?operator simplifies error propagationThese changes make the example more maintainable and easier to understand.
src/worldstate/fixtures/alert.rs (1)
3-5: Well-structured fixture with proper localization support.The fixture is well-structured using the custom
fixture!macro, with proper separation between different localized versions of the test data. This approach makes it easy to test localization features.Also applies to: 88-90
src/worldstate/fixtures/flash_sale.rs (1)
3-1151: Well-structured fixture data with comprehensive test cases.The flash sale fixture data is well-organized and provides a wide range of test cases with various types of items, prices, and date configurations. This will be useful for testing different scenarios and edge cases in the flash sale functionality.
src/worldstate/fixtures/event.rs (2)
3-84: Good baseline test data for event handling.The event fixture provides essential test cases for different event types with appropriate fields and values.
86-165: Good localization test coverage with different event description values.The second fixture variant shows changes to the description, tooltip, and node fields, which helps test localization and content updates. This is particularly valuable for ensuring the system can handle different text formats and content variations.
warframe-macros/src/model/struct_impl/return_style.rs (2)
9-13: Clean enum definition with descriptive variants.The
ReturnStyleenum has clear, concise variants that effectively represent the return style options.
15-37: Well-implemented Parse trait with robust error handling.The implementation properly handles different error cases with descriptive error messages. It correctly:
- Checks if the expression is a path
- Extracts the identifier
- Validates against allowed values
This ensures parsing errors are caught early with clear feedback to developers.
src/worldstate/fixtures/sortie.rs (2)
3-46: Good test fixture with comprehensive sortie details.This fixture includes all relevant sortie information with multiple mission variants, modifiers, and locations, providing good test coverage for the sortie functionality.
48-89: Excellent internationalization test case.The second fixture provides a great test case for internationalization support by including Chinese text for mission types, modifiers, and descriptions while maintaining English keys. This helps ensure the application correctly handles multi-language content.
src/worldstate/fixtures/invasion.rs (4)
1-899: The fixture provides comprehensive test data for invasionsThis new fixture file provides detailed mock data for invasions in both English and Chinese, with realistic field values that should cover various test scenarios.
28-28: CDN Resource Availability: Verify URLs and Implement FallbacksThe code references multiple image URLs from
cdn.warframestat.us. Ensure these resources are reliable and have appropriate fallbacks.Also applies to: 47-47, 85-85, 104-104, 146-146, 187-187, 228-228, 269-269, 310-310, 351-351, 392-392, 433-433, 474-474, 493-493, 531-531, 550-550, 593-593, 633-633, 674-674, 715-715, 757-757, 798-798, 839-839, 880-880
104-104:Details
❓ Verification inconclusive
Handle empty thumbnail URLs
This line contains an empty thumbnail URL. Ensure that your code properly handles missing thumbnail images.
🏁 Script executed:
#!/bin/bash # Check how empty thumbnail URLs are handled in the codebase # Search for patterns that might handle empty thumbnails rg -A 3 "thumbnail\s*==" --type rust rg -A 3 "thumbnail\.is_empty\(\)" --type rust rg -A 3 "thumbnail\s*!=" --type rustLength of output: 125
Action: Verify Empty Thumbnail Handling
Our initial search did not reveal explicit logic for handling empty thumbnail URLs across the codebase. Please manually verify that downstream processes consuming the
"thumbnail": ""value insrc/worldstate/fixtures/invasion.rseither accommodate an empty string gracefully or that a default image is provided. If additional handling is missing, consider adding a conditional check or a fallback mechanism.
155-156:Details
❓ Verification inconclusive
Verify negative completion percentages are handled correctly
Several invasion events have negative completion percentages. Ensure that the UI and any calculations involving these values handle negative percentages appropriately.
Also applies to: 196-197, 237-238, 278-279, 319-320, 360-361, 401-402, 642-643, 683-684, 724-725, 765-766, 806-807, 847-848
🏁 Script executed:
#!/bin/bash # Check how negative completion percentages are handled in the codebase # Search for code that processes the completion field rg -A 3 "completion" --type rust | grep -v "fixtures"Length of output: 749
Action Required: Verify Handling of Negative Completion Percentages
The fixture now provides a negative value (e.g.,
"completion": -0.273333333333325), and the model (src/worldstate/models/invasion.rs) merely declares this field as af32without explicit adjustments. Please double-check that:
- The UI renders negative completion percentages correctly.
- Any calculations using this field gracefully handle negative values.
- This verification extends to all related instances (lines 196-197, 237-238, 278-279, 319-320, 360-361, 401-402, 642-643, 683-684, 724-725, 765-766, 806-807, 847-848).
src/worldstate/fixtures/void_trader.rs (1)
1-39: Well-structured void trader fixture with localization supportThis fixture provides consistent test data for the void trader in both English and Chinese. The structure is clean and appropriate for testing various scenarios.
src/worldstate/models/reward.rs (1)
1-36: Good refactoring to attribute-based model definitionsThe transition from macro-based model definitions to explicit struct definitions with the
#[model]attribute enhances code readability and maintainability while preserving the same functionality.src/worldstate/fixtures/news.rs (3)
1-424: Comprehensive news fixture with localization supportThis fixture provides detailed test data for news items in both English and Chinese, which will be useful for testing localization features.
96-113: Missing news items in Chinese versionThe Chinese version of the fixture is missing two news items that are present in the English version (items with IDs "67ab9dd63ab8bf46df02e9b1" and "67ae4e9fca4611344608d246"). Verify if this inconsistency is intentional.
If this is intentional, consider adding a comment explaining why certain items are excluded from the Chinese version. If not, consider aligning the two datasets for more consistent testing.
Also applies to: 204-220
107-108:Details
❓ Verification inconclusive
Check handling of future-dated event
This news item has start and end dates in the future ("startDate" and "endDate"). Ensure that your code correctly handles events that haven't occurred yet, especially when calculating the "eta" field.
🏁 Script executed:
#!/bin/bash # Check how future dates are handled in the codebase # Search for code that processes date/eta fields rg -A 3 "startDate|endDate|eta" --type rust | grep -v "fixtures"Length of output: 5500
Verify Future-Dated Event Handling in News Fixtures
The fixture in
src/worldstate/fixtures/news.rsuses futurestartDateandendDatevalues. Please ensure that the downstream logic—specifically the computation of the "eta" field viaget_short_format_time_string(self.expiry())insrc/worldstate/models/base.rs—correctly handles events that have not yet occurred. In particular:
- Verify that computing
etafor future-dated events yields a proper, non-negative time difference.- Check that similar date-handling logic in related models (e.g., in
src/worldstate/models/invasion.rs) remains consistent.- Confirm (or add) unit tests to validate the behavior for events scheduled in the future.
src/worldstate/fixtures/deep_archimedea.rs (2)
47-48: Fix typo in the fixture JSON dataThere's a typo in the description for the "Postmortal Surges" risk variable:
- "description": "Slain enemies burts with Void energy." + "description": "Slain enemies burst with Void energy."This typo appears in both instances of the JSON (lines 47 and 140). While this won't affect functionality, it's good to maintain accuracy in game descriptions.
3-188: Code structure looks goodThe fixture is well-structured with consistent formatting. It properly uses the
fixture!macro to define test data for both English and localized versions, which is helpful for internationalization testing.src/worldstate/models/mission.rs (2)
1-11: Good refactoring from macro to attribute-based model definitionThe change from using a
model_builder!macro to the#[model]attribute macro is a good improvement. This approach is more idiomatic in Rust and makes the code easier to read and maintain.
12-70: Well-documented struct fieldsThe addition of documentation comments for each field in the
Missionstruct greatly improves code readability and maintainability. The comments clearly explain the purpose of each field, which will be helpful for future development and onboarding.src/worldstate/fixtures/steel_path.rs (1)
3-262: Well-structured fixture with localization supportThe fixture is well-structured and includes both English and Chinese translations, which is excellent for testing internationalization. The consistent formatting and organization of the JSON data make it easy to read and maintain.
src/worldstate/language.rs (3)
7-7: Good addition of ordering traits to Language enumAdding
PartialOrdandOrdtraits to theLanguageenum is beneficial as it allows for ordering and sorting operations on language values, which could be useful for displaying languages in a consistent order in UIs or for sorted collections.
36-47: Improved code clarity with explicit enum variant referencesThe change to explicitly reference enum variants with the
Language::prefix rather than using a wildcard import improves code clarity and maintainability. This makes it immediately clear where each variant comes from and avoids potential naming conflicts.
55-66: Consistent use of explicit enum variant referencesThe same improvement of using explicit
Language::prefixes has been consistently applied to this implementation as well, which is good for code consistency.src/worldstate/models/items/sentinel.rs (1)
8-8: Good addition of the Clippy lint suppression.The
#[allow(clippy::struct_excessive_bools)]attribute is appropriately added to suppress warnings about theSentinelstruct having many boolean fields. This is reasonable since this struct represents a game entity that naturally has multiple boolean properties by design.src/worldstate/fixtures/syndicate_mission.rs (2)
1-3: LGTM! Good use of the fixture macro.The fixture macro is used correctly to define test data for the syndicate mission system.
1082-1083: Nice internationalization support in fixture data.The fixture includes localized syndicate names in different languages, which is great for testing internationalization functionality. This ensures the application can properly display content in multiple languages.
Also applies to: 1304-1305, 1440-1441, 1452-1453, 1472-1473, 1492-1493, 1504-1505, 1516-1517, 1528-1529, 1548-1549, 1568-1569, 1580-1581, 1592-1593, 1640-1641, 1652-1653, 1664-1665, 1676-1677, 1688-1689, 1736-1737, 1748-1749, 1760-1761, 1780-1781, 1800-1801, 1812-1813
src/market/models/versions.rs (4)
5-5: Good use of the queryable macro.The macro implementation for
Versionsenables API querying against the "/versions" endpoint, which follows the pattern used elsewhere in the codebase for consistency.
7-14: Well-structured Versions model with proper serde configuration.The
Versionsstruct is properly defined with appropriate fields and serde attributes for camelCase JSON deserialization, which aligns with the API's response format.
16-23: Well-structured Apps model.The
Appsstruct correctly captures version information for different platforms with proper field naming.
42-56: Good test implementation using rstest.The test is well-structured using rstest for loading and testing the fixture data. This approach makes it easy to maintain and extend the test as needed.
src/market/models/fixtures/item.json (1)
1-35: Well-structured JSON fixture for item data.The JSON fixture for the "Mirage Prime Set" is well-structured and includes all necessary fields for testing item deserialization, including internationalization support. The data appears to be accurate and complete.
src/worldstate/fixtures/weapon.rs (2)
1-657: Great implementation of the ranged weapon fixture.The fixture provides comprehensive data for testing, including weapon statistics, attacks, components, and drop rates. The structure is well-organized and includes all necessary fields for proper testing.
659-973: Consider adding schema validation.The melee weapon fixture contains complex nested data structures. Consider adding JSON schema validation to ensure data consistency.
Example schema validation:
use schemars::JsonSchema; use serde::{Deserialize, Serialize}; #[derive(Debug, Serialize, Deserialize, JsonSchema)] struct WeaponAttack { name: String, speed: f64, crit_chance: f64, crit_mult: f64, status_chance: f64, damage: HashMap<String, f64>, #[serde(skip_serializing_if = "Option::is_none")] shot_type: Option<String>, #[serde(skip_serializing_if = "Option::is_none")] shot_speed: Option<f64>, #[serde(skip_serializing_if = "Option::is_none")] flight: Option<f64>, } #[derive(Debug, Serialize, Deserialize, JsonSchema)] struct Weapon { attacks: Vec<WeaponAttack>, // ... other fields }src/worldstate/fixtures/archon_hunt.rs (2)
1-59: LGTM: Well-structured archon hunt fixture.The English version of the fixture is well-structured and provides all the necessary data for testing interactions with the worldstate API.
60-113: Appropriate localization implementation.The Chinese translation maintains the same data structure while properly translating user-facing text fields like mission nodes, mission types, boss name, and faction. This provides good test coverage for the localization system.
src/worldstate/models/reward_type.rs (2)
1-6: Good refactor to use the model attribute macro.The switch from manual enum definitions to using the
#[model]attribute macro improves code maintainability and consistency with the rest of the codebase.
7-135: Well-documented enum with consistent naming.Each variant in the RewardType enum is properly documented with comments, and the naming is consistent throughout. The use of
#[serde(rename_all = "camelCase")]ensures proper serialization.src/worldstate/fixtures/fissure.rs (2)
1-636: Comprehensive fissure test fixture with good coverage of mission types.The English version of the fixture includes a wide variety of mission types, locations, and properties, providing excellent test coverage for the fissure functionality. The data structure is well-organized and includes both active and expired states.
637-1268: Well-implemented localization with consistent translations.The Chinese version maintains the same data structure while properly translating location names, mission types, and faction names. This provides good test coverage for both the base functionality and localization system.
src/worldstate/fixtures/nightwave.rs (1)
1-308: Well-structured fixture with comprehensive test data.The nightwave fixture is well-organized and provides comprehensive test data for both English and Chinese localizations, which will be valuable for testing internationalization support.
I particularly like the completeness of the fixture data, including various challenge types, activation states, and timestamps. This should provide good coverage for testing the nightwave functionality.
src/worldstate/models/damage_type.rs (3)
10-12: Good addition of derive_more::Display.Adding the
Displaytrait to theDamageTypeenum enhances its usability by allowing it to be formatted as a string easily.
49-64: Improved code quality with #[must_use] and explicit imports.The
#[must_use]attribute is a good addition as it prevents potential bugs from ignoring the return value. The explicit imports of enum variants also improve code readability by making it clear which variants are being used.
92-111: Enhanced documentation and code clarity.The improved documentation with explicit type references and the addition of
#[must_use]attribute make the API more user-friendly. The explicit imports significantly improve code readability.warframe-macros/src/lib.rs (1)
1-23: Well-documented procedural macro with clear structure.The
modelmacro is excellently documented with clear explanations of its purpose, arguments, and behavior. The implementation follows good practices by delegating to theexpandfunction and properly handling the conversion between differentTokenStreamtypes.The macro will simplify model definitions across the codebase by automatically applying common attributes and deriving appropriate traits based on whether the target is a struct or enum.
src/worldstate/fixtures/item.rs (1)
1-25: Well-defined fixture for a simple item.The
item_sigilfixture is concise and effectively represents a simple game item with its properties.src/market/error.rs (1)
1-21: Improved error handling structure - LGTM!The refactoring from
ApiErrorto a more idiomaticErrorenum is a good improvement. The use of#[error(transparent)]for the reqwest error and explicit error messages for other variants enhances error clarity. The documentation comments are clear and comprehensive.src/market/models/item_short.rs (2)
5-40: Well-structured ItemShort implementation with comprehensive fieldsThe
ItemShortstruct is well-designed with appropriate field types and documentation. The optional fields are correctly modeled usingOption<T>types, which aligns with Rust best practices.
42-53: Complete and consistent derivations for ItemShortI18NGood job implementing the
ItemShortI18Nstruct with appropriate trait derivations, especially includingHash,Eq, and ordering traits which will enable its use in collections and sorting operations.src/worldstate/models/steel_path.rs (1)
1-25: Good transition from macro-based to attribute-based model definitionsThe transition to using the
#[model]attribute makes the code cleaner and more maintainable. The#[serde(rename = "currentReward")]attribute is correctly applied for JSON serialization/deserialization compatibility.src/worldstate/models/syndicate.rs (2)
42-43: Add missing serde rename attribute for Entrati variantThe Entrati variant is missing the
#[serde(rename = "...")]attribute that all other variants have. This may cause deserialization issues if the API uses a different string representation./// Entrati + #[serde(rename = "Entrati")] Entrati,
53-54: Add missing serde rename attribute for Necraloid variantThe Necraloid variant is missing the
#[serde(rename = "...")]attribute that all other variants have. This may cause deserialization issues if the API uses a different string representation./// Necraloid + #[serde(rename = "Necraloid")] Necraloid,src/worldstate/models/alert.rs (1)
22-45: Nice test refactoring with rstest!The test module has been nicely refactored to use rstest for fixture-based testing instead of making actual HTTP requests. This approach is more maintainable and consistent with the testing pattern used in other model files.
warframe-macros/src/model/enum_impl.rs (1)
16-16:Details
✅ Verification successful
Ensure derive_more crate is available
The code unconditionally adds
derive_more::Displayto the derive list. Ensure that thederive_morecrate is included in the dependencies of any crate using this macro.
🏁 Script executed:
#!/bin/bash # Check if derive_more is listed in dependencies grep -r "derive_more" --include="Cargo.toml" .Length of output: 114
Derive More Dependency Verified
The derive_more crate is confirmed in Cargo.toml (listed as
derive_more = { version = "2.0.1", features = ["full"] }), so the inclusion ofderive_more::Displayin the derive list is valid.devtools.nu (1)
30-40: Add language validation toworldstate_reqfor consistencyUnlike
market_req, theworldstate_reqfunction doesn't validate the language parameter against the supported languages list. For consistency and to prevent errors, consider using the same validation approach.export def worldstate_req [ endpoint:string, # The endpoint to request - language?:string # The language to request the endpoint in + language?:string@languages # The language to request the endpoint in ]: nothing -> table { if language == null { return (http get $"https://api.warframestat.us/pc($endpoint)") } else { return (http get $"https://api.warframestat.us/pc($endpoint)?language=($language)") } }src/worldstate/models/orb_vallis.rs (4)
1-2: Import of new macro package looks goodThe import of the
warframe_macros::modelis consistent with the broader refactoring to use attribute-based model definitions instead of macro-based ones.
3-11: Clean enum definition with appropriate serializationThe
OrbVallisStateenum is well-defined with proper documentation. The#[serde(rename_all = "lowercase")]attribute ensures correct serialization format.
13-21: Well-structured struct with clear model attributesThe
OrbVallisstruct definition with the#[model]attribute is clear and includes appropriate parameters for the endpoint, return style, and timing characteristics.
24-47: Good transition to fixture-based testingThe test module has been successfully refactored to use
rstestwith fixtures instead of making actual HTTP requests. This approach is more reliable and faster for unit testing.I notice the same testing pattern is used across multiple files in the codebase, providing good consistency.
src/worldstate/models/syndicate_mission.rs (3)
5-30: Well-structured SyndicateJob model with proper field renamingThe
SyndicateJobstruct is well-defined with clear documentation for each field. The use of#[serde(rename = "type")]forjob_typeand#[serde(rename = "minMR")]forminimum_mrensures compatibility with the API's JSON format while maintaining idiomatic Rust naming conventions.
32-50: Clear SyndicateMission model definition with appropriate endpointThe
SyndicateMissionstruct is well-defined with the correct model attributes, including the endpoint path, return style, and timing flag. The documentation includes a helpful note about possible empty responses.
53-76: Consistent test implementation using fixturesThe test module follows the same pattern as other model tests in the codebase, using rstest and fixtures to test deserialization. This approach is more maintainable and consistent across the project.
src/worldstate/models/event.rs (3)
1-7: Clean imports and organizationGood use of the model macro and well-structured imports for related types (Faction, Reward, Syndicate).
9-53: Well-designed Event struct with appropriate optionalsThe
Eventstruct is well-defined with clear documentation and appropriate use ofOption<T>for fields that might be absent in the API response. The model attributes correctly specify the endpoint, return style, and timing characteristics.
56-80: Consistent testing patternThe test module follows the same pattern used throughout the codebase, with rstest fixtures for both English and multilingual tests, promoting consistency and maintainability.
src/worldstate/models/deep_archimedea.rs (4)
1-10: Well-structured DeepArchimedea modelThe
DeepArchimedeastruct is properly defined with model attributes. Note that themissionsfield uses a fixed-sized array of exactly 3 elements, which is uncommon but likely reflects the game's design.
12-18: Good use of raw identifier for reserved keywordThe
DeepArchimedeaMissionstruct correctly uses a raw identifier (r#type) to avoid conflicts with the Rusttypekeyword, while maintaining the proper serialization name with#[serde(rename = "mission")].
20-25: Simple and clear modifier structureThe
DeepArchimedeaModifierstruct is well-defined with clear field names.
67-92: Consistent test implementationThe test module follows the same pattern as other model tests in the codebase, using rstest with fixtures. This consistency is excellent for maintainability.
src/worldstate/models/news.rs (9)
5-5: No issues with importing the model macro.
This import statement correctly referenceswarframe_macros::model.
7-9: Attribute-based model definition looks good.
The doc comment and the#[model(endpoint = "/news", return_style = Array)]attribute are consistent with the rest of the codebase.
13-13: Well-documented fields.
Each field has a concise doc comment, which improves readability and maintainability.Also applies to: 16-16, 19-19, 22-22, 25-25, 28-28, 31-31, 34-34, 37-37
42-44: Test module definition is clear.
Declaring a separatetest_newsmodule with the#[cfg(test)]guard is a good practice.
48-48: Queryable import is appropriate.
No issues noted with importing theQueryabletrait forNews.
49-51: Fixtures usage is correct.
Bringing innewsandnews_enfixtures helps maintain test clarity.
55-55: Type alias is consistent.
The aliastype R = <News as Queryable>::Returnmakes the test usage straightforward.
57-57: Use of rstest is fine.
Annotating tests with#[rstest]keeps each scenario concise.
62-62: Second rstest block is also good.
It follows the same pattern, which keeps the tests consistent.src/worldstate/models/faction.rs (4)
1-1: Macro import is valid.
No issues importingwarframe_macros::model.
11-11: Applying#[model]to the enum.
This is aligned with the new attribute-based approach in the codebase.
39-39: Use of#[must_use]is appropriate.
This ensures the return value of the function isn't accidentally ignored.
83-83: Another#[must_use]annotation is consistent.
Same reasoning applies as above: ensuring return values aren't discarded.src/worldstate/models/flash_sale.rs (15)
1-1: Valid macro import.
No issues noted here.
3-3:ItemStringWrapperimport is used effectively.
Ensures typed representation for theitemfield.
5-5: Good doc comment for struct definition.
Provides a quick description for flash sales.
6-7:#[model(..., timed)]usage is correct.
Enabling thetimedparameter automatically managesactivation&expiryfields in the macro expansions.
8-9: Field type changed toItemStringWrapper.
This should help parse item representations more consistently.
11-12: Discount field is well documented.
No issues here.
14-15: Platinum price field.
Straightforward integer field.
17-18: Credit price field.
Also clear and fits the structure.
20-21: Optional popularity flag.
Great usage ofOption<bool>for uncertain state data.
23-24: Optional featured flag.
Continuing with consistent usage ofOption<bool>.
28-30: Test module with#[cfg(test)],#[rstest].
No concerns; consistent naming.
34-37: Importing Queryable and fixture data.
Ensures easy test setup and fosters clear, maintainable tests.
41-41: Type alias aligns with the Queryable trait.
Keeps test code succinct.
43-43: Appropriate rstest annotation.
Facilitates parameterized testing.
48-48: Second rstest block is well structured.
Continuing the same logic for multilingual tests.warframe-macros/src/model/struct_impl/mod.rs (3)
1-3: Module declarations are straightforward.
Splitting logic intoargsandreturn_stylesubmodules aids organization.
4-28: Import statements are appropriate for macro processing.
They cleanly structure references toargs,syn,quote, etc.
56-119:parse_structfunction effectively handles struct transformations.
This function handles:
- Prepending serializer attributes.
- Adding optional
timedfields.- Implementing traits if requested.
Overall, the approach is solid and leverages Rust’s macro ecosystem well.src/worldstate/models/cetus.rs (8)
1-2: Use of custom macros
Importingwarframe_macros::modelis consistent with similar patterns in the codebase, and there's no immediate concern here.
3-10: EnumCetusStateis well-defined
Renaming variants to lowercase aligns nicely with JSON serialization conventions, and the doc comments help clarify the enum.
13-21: StructCetusdefinition
Applying#[model(endpoint = "/cetusCycle", return_style = Object, timed)]is straightforward, and the documentation is helpful for maintainers.
23-26: Test module creation
Adoptingrstestaligns with other parts of the codebase and helps maintain a consistent testing approach.
29-35: CombiningQueryablewith fixtures
Referencingfixtures::cetusis standard and poses no issues.
36-37: Type alias clarity
Settingtype R = <Cetus as Queryable>::Return;makes the test code more readable and explicit.
38-41: Test coverage suggestion
Currently, the test checks only for successful deserialization. Consider validating specific fields for stronger test coverage.
43-46: Test coverage suggestion
Similarly, verifying more than mere parsing success would help detect potential regressions.src/worldstate/models/construction_progress.rs (7)
1-3: Basic imports
Importingwarframe_macros::modeland referencing the local deserialization helper is consistent with the existing approach.
5-10: Model definition
#[model(endpoint = "/constructionProgress", return_style = Object)]aligns with overall design patterns, and custom f32 deserialization makes sense for external data.
22-25: Test module definition
Adoptingrstestandserde_json::from_strhere mirrors other test modules and looks fine.
28-31: Fixture references
Referencingfixtures::construction_progressis straightforward. No concerns identified.
35-36: Type alias usage
DefiningRas<ConstructionProgress as Queryable>::Returnis a neat tactic for readability.
37-39: Test coverage suggestion
This test only checks for successful parsing. Assertions on field values would bolster confidence in correctness.
42-44: Test coverage suggestion
Similar to the above, adding assertions on expected values aids in detecting regressions.src/worldstate/models/daily_deal.rs (7)
1-3: Imports
Bringing inwarframe_macros::modelandItemStringWrapperis coherent with the rest of the codebase.
5-28: StructDailyDeal
The#[model(endpoint = "/dailyDeals", return_style = Array, timed)]attribute is appropriate. Field documentation is clear, making it easy to maintain.
31-34: Test module introduction
Usage ofrstestand JSON deserialization is consistent with other testing patterns.
37-41: Fixture references
Pulling test data fromfixtures::daily_dealis straightforward and effective for these tests.
44-44: Type alias
type R = <DailyDeal as Queryable>::Return;nicely clarifies the test’s return type.
46-49: Test coverage suggestion
Currently, the test only verifies successful deserialization. Consider asserting field values for a more robust check.
51-54: Test coverage suggestion
Likewise, verifying actual field contents can prevent unnoticed data or structural changes.src/market/models/item.rs (6)
1-3: Imports
Usingserde::Deserializeand referencingsuper::i18n::I18Nis a standard setup.
5-7: Excessive booleans
There are several booleans within theItemstruct. Refactoring them into an enum or descriptive type could improve clarity and memory usage.
9-38: ComprehensiveItemstruct
This covers a wide range of fields for items, with appropriate optional types for uncertain data. No further issues spotted.
40-49:ItemI18Nstruct for localization
Optional fields are used appropriately, and the design is flexible enough to accommodate missing data.
53-55: Test module setup
Imports and references toResponseBasematch the established pattern for item deserialization tests.
57-66: Test coverage suggestion
This test ensures only that parsing does not fail. Adding checks for field values would enhance confidence in the item data model.src/worldstate/models/fissure.rs (2)
1-23: Great use of explicit enum definitions and documentation.
The transition from macro-based declarations to a standard Rust enum with#[model]annotations makes the code more transparent. Defining numeric discriminants (Lith = 1, etc.) is valid but a bit unusual; ensure these values align with expectations in the serialization/deserialization layer.
25-63: Well-structured struct definition and field-level documentation.
The fields are clearly named and annotated with#[serde(rename = "...")], which is good for bridging field names with external JSON keys. If there’s a known default or fallback value fortier_nameortier, consider making fields optional or usingDefaultif appropriate. Otherwise, the explicit nature of these fields is beneficial for highlighting required data.src/worldstate/models/cambion_drift.rs (2)
1-14: Clear definition of the state enum with inlined documentation.
Using#[model]with#[serde(rename_all = "lowercase")]ensures that the enum variants match the JSON format. The doc comments effectively describe each variant. This pattern is consistent and should minimize confusion.
16-49: Straightforward struct modeling.
Fields likeidandstateare well documented, making it apparent what each represents. No obvious issues with type usage or naming. If the Cambion Drift states expand in the future, you’ll likely need to update theCambionDriftStateenum accordingly.src/worldstate/utils.rs (2)
1-8: Enum for change representation is succinct and self-explanatory.
The variantsAddedandRemovedare well-documented and will be useful for signifying how elements differ between two collections.
10-14: Struct fields are clearly referenced and typed.
DefiningCrossDiffwith slices is a clean approach. Prefer private struct fields if users are expected to interact mainly through the associated methods, butpub(crate)is acceptable here given the file context.src/worldstate/mod.rs (3)
3-4: Comprehensive Quickstart code comments.
These doc lines effectively illustrate usage viaClientandError. The inline demonstration is in line with standard Rust doc practices.Also applies to: 7-14, 17-18
29-30: Well-organized module exports.
Makingmodelsprivate while re-exporting specific items inqueryablekeeps the top-level namespace clean.pub(crate) mod fixturesfor tests is also a practical choice. This structure fosters maintainability.Also applies to: 34-35, 37-61
63-69: Convenient re-exports for external users.
Re-exporting large sets of public types (e.g.,Fissure,Cetus,Error) simplifies adoption. The final lines re-exporting themodelmacro is a nice touch, promoting reusability. No concerns here.Also applies to: 70-109, 110-114
src/worldstate/models/invasion.rs (2)
39-41: Good use of Serde rename.
Renaming to"desc"might prevent confusion if the external API uses that field name.
12-13:Details
✅ Verification successful
Confirm the
#[model]attribute usage.
Ensure that the#[model]attribute is properly recognized by your macro setup so it can generate the required code.
🏁 Script executed:
#!/bin/bash # Searching for the implementation of the `#[model]` macro usage # We'll look for its definition in `warframe_macros` as indicated by the snippet. ast-grep --pattern $'#[model]'Length of output: 3347
Macro usage confirmed in
src/worldstate/models/invasion.rs:
The#[model]attribute on line 12 (and its parameterized counterpart on line 25) is consistent with its usage across other modules (e.g., invoid_trader.rs,fissure.rs, etc.). This indicates that your macro setup is correctly recognizing and handling the#[model]attribute. No further changes are required.src/market/mod.rs (8)
4-5: Feature gating is well-structured.
Using#[cfg(feature = "market_cache")]is a clean approach to conditionally compile caching logic.
8-8: Restrictingmodelstopub(crate)aligns with domain-driven design.
This ensures internal components remain encapsulated and fosters a cleaner public API.
10-20: Neat reexports.
Re-exportingClient,Error, andResulthelps external code consume the API more naturally. No concerns here.
26-34: Centralizing queryable models is a good architecture choice.
Having a separatequeryablemodule re-export the relevant parts frommodelsclarifies what is intended for public queries. Nicely done.
38-42: Robust newtype Slug.
Great approach to standardize input withToSnakeCase. The derivations likeDisplay,Into, andAsRefmake it flexible. Overall, well-structured.
63-79: PartialEq implementations look correct.
This is a practical approach for ensuring thatSlugvalues can be compared directly to standard strings.
82-91: Unit tests are straightforward and valuable.
They verify the slug conversion comprehensively for typical examples.
21-24:Details
❓ Verification inconclusive
Confirm the necessity of re-exporting
QueryableandResponseBase.
Confirm these are intended to be consumed externally. It's fine if this is deliberate, but be mindful of your public API surface.
🏁 Script executed:
#!/bin/bash # Verify external usage of Queryable or ResponseBase. rg -A 3 "use .*::Queryable"Length of output: 29
Attention: Verify the necessity of these re-exports for your intended public API
The initial search didn’t return any internal references for either
QueryableorResponseBase. This suggests they might solely be exposed for external consumers. Please confirm that these re-exports are indeed meant to be part of your public API. If they aren’t necessary externally, consider removing or limiting their visibility.
- Location:
src/market/mod.rs(lines 21–24)- Action: Manually verify the intended external consumption or provide documentation on their purpose.
src/market/models/mod.rs (5)
1-5: Fine-grained module splitting.
Exposing each of these submodules as top-level is a solid approach to keep the code organized.
18-19: Synchronization of Serde rename strategies.
#[serde(rename_all = "camelCase")]is good if the API expects that form. Confirm that any server-side or related code also consistently uses camelCase to avoid mismatch.
20-24: HelpfulResponseBasestructure.
HavingdataanderrorasOption<T>is flexible. Just ensure each usage properly checks forerrorordatapresence, as you do inquery.
26-28: Use ofQueryabletrait is consistent with the design.
The associatedDatatype, combined withendpointconstants, forms a clear contract for fetching. Good use ofDeserializeOwned.
53-68: Macro-based convenience is well-designed.
impl_queryable!cleanly sets up either an array or object return style. This is a flexible pattern for expansions to new endpoints.src/worldstate/models/void_trader.rs (4)
1-7: Excellent modernization from macros to attribute-based approach!The transition from the
model_builder!macro to the#[model]attribute makes the code more maintainable and readable while reducing macro magic. The new approach provides better IDE support and clearer error messages.
8-9: Good type refinement with ItemStringWrapperChanging from
StringtoItemStringWrapperfor the item field improves type safety and adds domain-specific validation or behavior that might be required for item strings in the game context.
18-20: Clean attribute parameters for model definitionThe
#[model(endpoint = "/voidTrader", return_style = Object, timed)]attribute clearly specifies the API endpoint and return style, which improves clarity about how this model is used in API interactions.
32-46: Improved test structure with rstestThe transition to using
rstestand thefrom_strapproach for testing is more straightforward and focused than previous client-based tests. This approach isolates the deserialization logic for testing, which makes tests more predictable and faster.src/worldstate/models/sortie.rs (4)
5-20: Good use of documentation comments with the model attributeThe documentation comments above each field are clear and informative, making the purpose of each field immediately evident. The
#[model]attribute is properly used for theSortieMissionstruct.
22-37: Appropriate model attribute configuration and explicit serde renamingThe
#[model]attribute with the endpoint, return style, and timed parameters is well-defined. The explicit#[serde(rename = "variants")]annotation on themissionsfield clearly indicates how the field is serialized/deserialized, which is important for API compatibility.
40-54: Well-structured test module with type aliasThe test module is well-structured with a type alias for the return type, which simplifies the test code. The
rstestusage allows for parameterized testing with different fixtures.
55-58:Details
❓ Verification inconclusive
Verify unwrap safety in tests
The test is using
unwrap()which will panic if deserialization fails. While this is common in tests, ensure your test fixtures are comprehensive and represent all possible valid responses to avoid false test failures.
🏁 Script executed:
#!/bin/bash # Check if the test fixtures include all required fields for the Sortie model echo "Examining sortie test fixtures..." rg -A 3 'pub struct Sortie' src/worldstate/models/sortie.rs echo "Checking for required fields in test fixtures..." fd -e rs -p "*fixtures*sortie*" | xargs cat | grep -o '"[^"]*"' | sort | uniqLength of output: 824
Verify and Enhance Unwrap Safety in Sortie Model Tests
- The test in
src/worldstate/models/sortie.rs(lines 55–58) usesunwrap()for deserialization. While this is acceptable in tests, please double-check that your test fixtures fully represent all required fields defined for the Sortie model (e.g.,Sortie.id) and related types such asSortieMission.- The automatic check for fixture completeness encountered an error with the search command (regex error with the
fdpattern). Consider updating the command to use--fixed-stringsor--globfor literal or glob-based matching so that you can confidently verify that your fixtures include all necessary fields.src/worldstate/models/archon_hunt.rs (4)
1-6: Good import organizationThe imports are well-organized, with the model macro imported first, followed by the domain-specific imports. This makes the code easy to read and maintain.
8-37: Clear and thorough documentation of ArchonHuntMission fieldsEach field in the
ArchonHuntMissionstruct is well-documented with clear descriptions of its purpose, which enhances code readability and maintainability.
39-59: Well-defined model with appropriate attributesThe
ArchonHuntstruct uses the model attribute with appropriate parameters, clearly specifying the endpoint and return style. The fixed-size array for missions ([ArchonHuntMission; 3]) matches the game's mechanics of having exactly 3 missions per hunt.
76-85: Simple, effective test implementationThe tests are concise and focus on the core functionality: deserializing JSON into the model. This approach is easier to maintain than more complex test setups.
src/worldstate/models/arbitration.rs (5)
21-35: Improved error handling in deserialize_expiryThe refactored error handling using method chaining with
mapandor_elseis more idiomatic Rust and improves readability while maintaining the same functionality. The pattern matching on error kinds is also well-structured.
37-44: Clear model attributes with custom deserializationThe
#[model]attribute declaration clearly specifies the model's properties, including the custom deserializer for theexpiryfield. This approach is more maintainable than using macros.
45-72: Well-documented structure with clear field purposesEach field in the
Arbitrationstruct has a descriptive documentation comment, and the serialize/deserialize annotations are explicit where needed, making the mapping between the API and the model clear.
76-77: Good use of #[must_use] attributeThe
#[must_use]attribute on theis_validmethod ensures that callers don't accidentally ignore the return value, which could lead to logic errors. This is a good defensive programming practice.
83-106: Consistent test structure across modulesThe test module follows the same pattern as the other model test modules, using
rstestfor parameterized testing and focusing on straightforward deserialization tests. This consistency makes the codebase easier to understand and maintain.warframe-macros/src/model/struct_impl/args.rs (7)
1-21: Imports are concise and tailored to parsing and macro generation needs.
No concerns: they align withsyn,quote, and other procedural macro dependencies.
25-28: Visibility note forQueryableParams.
The fieldsendpointandreturn_styleare private. If they need to be accessed externally, consider making them publicly accessible or providing getters. Otherwise, this is a solid encapsulation choice.
30-33: Structure for generating optional code blocks is clear.
TheOption<TokenStream>fields inQueryableImplallow flexible generation steps. This is a neat approach.
35-57: Endpoint implementation generation looks good.
This function produces a coherentEndpointtrait impl, with distinct logic for a static English endpoint and a variable language. The string formatting is straightforward and correct.
59-70: Queryable trait impl generation is well-structured.
Switching onReturnStyleto either array or object is a clear approach. Potential future extension might handle more styles if needed.
72-82: Combining endpoint & queryable traits is cohesive.
try_from_queryable_paramsmerges the two partial impl blocks. The approach is simple and effective.
84-186: Parsing logic is robust and well-documented.
- Validates
endpointformatting (must start with “/”).- Ensures
endpointandreturn_styleappear together.- Properly checks
timedusage withexpiryandactivationattributes.
Error messages are clear and informative. Overall, it’s well-structured parse logic.src/worldstate/models/nightwave.rs (5)
1-16: Adoption of the#[model]attribute for the enum is tidy.
The doc comments are consistent with Rust doc standards, and the introduction ofNightwaveChallengeTypevia#[model]clarifies usage.
18-43:NightwaveChallengestruct with#[model(timed)]is clear.
Fields are well-documented, and the rename ofdesctodescriptionis properly handled with#[serde(rename = "desc")].
46-65:#[must_use]onchallenge_typeenforces usage of the result.
The internal matching logic for difficulty levels is straightforward. The final fallback toUnknownis a good safety net.
67-88:Nightwavestruct with typed endpoint and timed attributes.
Leveraging#[model(endpoint = "/nightwave", return_style = Object, timed)]is consistent with the macro approach. The fields are also thoroughly documented.
90-115: Tests userstestand appear structurally sound.
Deserializing JSON fixtures into the generic return type ensures the macro logic is tested. The test names reflect the data they validate.src/worldstate/models/items/weapon.rs (4)
12-12: Updated import reference is appropriate.
Usingcrate::worldstate::models::damage_type::DamageType;provides clarity and consistency within the module structure.
41-41: Clippy allowance forstruct_excessive_boolsis justified.
Large game data structures often naturally contain many booleans. If maintainers find it manageable, suppressing this lint is acceptable.Also applies to: 159-159
122-122: Added doc comments referencingis_primeare consistent.
They clarify thevaultedfield’s purpose for prime items. This is a small but helpful documentation improvement.Also applies to: 229-229
345-376: New test module usingrstestis well-structured.
- Parsing logic with
serde_json::from_strensures correctness.- Matching
Weapon::Ranged(_)andWeapon::Melee(_)is straightforward.
No immediate issues found.src/worldstate/fixtures/mod.rs (1)
1-22: Modules for fixture submodules are consistent.All the declared submodules (alert, arbitration, etc.) are neatly organized and named. No issues found.
src/market/client.rs (22)
1-3: Imports for builder, status codes, and deserialization.Using
derive_builder,reqwest::StatusCode, andserde::de::DeserializeOwnedis a sound approach. No issues found.
4-15: Conditional imports for caching are properly gated.The use of
#[cfg(feature = "market_cache")]prevents compilation errors when this feature is disabled. Good job.
16-23: Conditional imports for rate-limiter.Gating the governor rate limiter behind
feature = "market_ratelimit"looks correct.
25-38: Reorganized imports for error types and query utilities.The consolidated imports for
Error,Queryable, andResponseBasehelp maintain clarity and keep the code well-structured.
43-44: Slugs type alias looks fine.Defining
type Slugs = Arc<HashSet<String>>;for read-only sets of slugs is a neat approach.
88-92:Defaultimplementation delegates toSelf::new().Neat approach for default behavior. No immediate concerns.
101-104: Builder factory method is correct.Providing a
builder()function to customize the client is a recognized best practice.
106-122: Cache retrieval logic is straightforward.The
get_from_cachemethod correctly downcasts data to the desired type while logging cache hits. This is well-structured.
124-131: Cache insertion is clear.
insert_into_cachestraightforwardly logs and inserts the item into the cache. No performance or correctness concerns.
133-143: Check for more robust error handling on non-2xx responses.
fetch_from_apireturns the rawreqwest::Error. In some scenarios, it might be beneficial to explicitly handle 4xx/5xx errors. Ensure the calling code either checks or interprets these appropriately.
145-151: Defaulting fetch to English is consistent.
fetch<T>simply callsfetch_using_languagewith English. This is a clean design choice.
153-161: Priority to English infetch_itemis consistent.Mirroring the logic of
fetch<T>for items is straightforward.
163-187:fetch_using_languagewith caching is well-structured.Good approach: the function tries the cache first, then queries and inserts, ensuring minimal repeated calls.
189-202: Graceful handling of 404 infetch_item_using_language.Returning
Nonefor 404 fosters clarity and aligns with the optional item semantics.
204-224: Set items retrieval logic is clear.
set_items_ofproperly documents how sets are returned. Nicely structured.
226-256: Well-structuredtry_get_itemfunction.Handles caching, 404 detection, and API error responses effectively.
258-289: Cached retrieval of item listings is effective.Storing items in
Arc<[ItemShort]>is a space-efficient approach for shared usage.
291-314:get_slugsderived fromitemsis logical.Building a set of item slugs from the existing item cache is a good approach.
316-326:is_slug_validleverages the slug cache.This simple membership check is efficient and readable.
328-333: Invalidating items resets slugs cache too.Makes sense to invalidate both caches together for reliability.
336-341:ratelimit!macro usage.Conditionally waits for the rate limiter, preventing excessive requests. Straightforward to read.
343-344:use ratelimit;ensures macro visibility.No issues with scoping or macro resolution.
src/worldstate/models/base.rs (5)
14-14: Newly importedDeserializeOwned.Allows generic deserialization in
Queryableimplementations. Looks appropriate.
17-17: UsingLanguagefromcrate::worldstate::language.Neat direct import clarifies the associated trait usage. No concerns.
79-79: Switch toErrortype inQueryableand top-level usage.Replacing the older
ApiErrorwithErrorunifies error handling. Ensure that all references toApiErrorare removed or updated.Also applies to: 94-95, 159-159
145-145: Space-separated time format.Appending a space after each computed component (
{div_time}{suffix}) is consistent with the final trimmed result. Implementation is straightforward.
155-155: Enforcing return usage with#[must_use].Tagging
fn opposite(&self) -> Selfwith#[must_use]helps catch logical errors if the result is accidentally ignored.src/worldstate/client.rs (14)
7-21: LGTM: Good import structure and organizationThe updated imports properly organize the necessary components and maintain a clean separation of concerns. The change from
ApiErrorto a more genericErrortype aligns with Rust's best practices for error handling.
23-48: LGTM: Improved documentation with clearer examplesThe updated documentation correctly reflects the new module structure and error handling approach, making it easier for users to understand how to use the library.
55-58: Good use of #[must_use] attributeAdding the
#[must_use]attribute to thenewmethod is a good practice as it warns users when they ignore the returnedClientinstance. The simplification of the implementation to directly returnSelf::default()improves readability.
86-91: LGTM: Consistent error type usageThe change from
ApiErrortoErrorin the method signature provides a more consistent error handling approach throughout the codebase.
118-123: LGTM: Consistent error type usageThe change from
ApiErrortoErrorin the method signature provides a more consistent error handling approach throughout the codebase.
147-153: LGTM: Consistent error type usageThe change from
ApiErrortoErrorin the method signature provides a more consistent error handling approach throughout the codebase.
180-191: LGTM: Consistent error type usageThe change from
ApiErrortoErrorin the method signature and the simplification of theLanguageimport provides a more consistent approach throughout the codebase.
193-212: Good refactoring of DummyCategory scopeMoving the
DummyCategorystruct inside thequery_by_urlmethod is a good practice as it encapsulates the struct within its only usage scope, reducing namespace pollution.
262-270: Good migration to structured loggingThe migration from
log::debug!totracing::debug!with structured fields improves the logging capabilities by providing more context in a structured format.
286-290: Good migration to structured loggingThe migration from
log::debug!totracing::debug!with structured fields improves the logging capabilities by providing more context in a structured format.
349-391: Consistent migration to Error type and structured loggingThe change from
ApiErrortoErrorin the method signature and the migration to structured logging withtracing::debug!are good practices that improve error handling consistency and logging capabilities.
371-375: CrossDiff implementation remains O(n²)As noted in previous review comments, the current implementation of
CrossDiffhas O(n²) complexity due to linear searches with.contains(). While this may be acceptable for small collections (as mentioned in previous comments), it's worth considering the optimization for future scalability.#!/bin/bash # Check if the O(n²) issue has been addressed in the CrossDiff implementation rg -A 5 -B 5 "pub fn new" src/worldstate/utils.rs
550-591: LGTM: Consistent error type usage and structured loggingThe change from
ApiErrortoErrorin the method signature and the migration to structured logging withtracing::debug!are good practices that improve error handling consistency and logging capabilities.
573-574: CrossDiff implementation remains O(n²)As noted in previous review comments, the current implementation of
CrossDiffhas O(n²) complexity due to linear searches with.contains(). While this may be acceptable for small collections (as mentioned in previous comments), it's worth considering the optimization for future scalability.src/worldstate/models/items/mod.rs (5)
3-24: LGTM: Improved module organization with public exportsMaking the module exports public (
pub use) instead of private is a good practice that improves the API's usability by allowing users to directly access the types they need without having to know the exact module structure.
27-42: LGTM: Proper module visibilityChanging the module declarations to private (
modinstead ofpub mod) while making the types public throughpub useis a good practice that allows for better encapsulation of implementation details while still providing access to the necessary types.
130-131: LGTM: Enhanced derive traits for Category enumAdding the
Copy,PartialOrd,Ord, andHashtraits to theCategoryenum enhances its usability, particularly for collections likeHashSetand sorting operations.
170-171: Improved documentation with backticks for code referencesAdding backticks around
ArchGunandArchMeleein the documentation comments properly indicates that they are code references, improving the documentation's clarity.Also applies to: 178-179
252-279: LGTM: Modern test structure with rstestThe refactoring of tests to use the
rstestframework improves test organization and readability. The direct testing ofmap_category_to_itemwithout involving a client query simplifies the test and makes it more focused on the functionality being tested.src/worldstate/models/mod.rs (5)
14-32: LGTM: Updated documentation with correct importsThe documentation examples have been correctly updated to use the new error type and import paths, providing clearer guidance for users.
34-63: LGTM: Improved module visibilityMaking the module declarations public (
pub modinstead ofmod) is a good practice that improves the API's usability by allowing users to directly access the modules they need.
65-71: LGTM: Clean import organizationThe updated imports properly organize the necessary components and maintain a clean separation of concerns.
73-130: Well-designed ItemStringWrapper with comprehensive methodsThe introduction of the
ItemStringWrapperstruct with methods for querying items provides a convenient abstraction for working with item strings. The comprehensive methods and documentation enhance usability.
132-136: LGTM: Appropriate implementation of AsRefImplementing
AsRef<str>forItemStringWrapperis a good practice that allows the wrapper to be used in contexts that expect a string reference, enhancing its interoperability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/market/client.rs (2)
4-15:⚠️ Potential issueConditional imports might cause compilation errors.
The conditional imports of
ItemShortandArcbehind themarket_cachefeature flag can cause compilation errors if these types are referenced outside the conditionally compiled blocks.Consider moving essential imports out of the feature-gated block:
#[cfg(feature = "market_cache")] use { - super::ItemShort, - super::cache::CacheKey, - moka::future::Cache, - std::{ - any::Any, - collections::HashSet, - sync::Arc, - time::Duration, - }, + super::cache::CacheKey, + moka::future::Cache, }; +use super::ItemShort; +use std::{ + any::Any, + collections::HashSet, + sync::Arc, + time::Duration, +};
97-99:⚠️ Potential issueAvoid unwrap() in library code.
Using
unwrap()in thenew()method can lead to panics at runtime, which is not appropriate for library code.pub fn new() -> Self { - ClientBuilder::default().build().unwrap() + ClientBuilder::default().build().expect("Failed to build Client") }Even better would be to return a Result:
-pub fn new() -> Self { - ClientBuilder::default().build().unwrap() +pub fn new() -> Result<Self> { + Ok(ClientBuilder::default().build()?) }
🧹 Nitpick comments (3)
README.md (1)
3-3: API Documentation Clarity
The updated introductory sentence now references both the Worldstate API and the warframe.market API with direct links, which improves discoverability. Please ensure the URLs remain valid over time.src/market/models/versions.rs (1)
25-40: Consider consistent spacing in struct fieldsThe
VersionCollectionsstruct has blank lines between fields, which is inconsistent with the compact style used in the other structs in this file. Consider removing these blank lines for consistency.#[derive(Debug, Clone, PartialEq, Deserialize)] pub struct VersionCollections { pub items: String, - pub rivens: String, - pub liches: String, - pub sisters: String, - pub missions: String, - pub npcs: String, - pub locations: String, }src/market/client.rs (1)
292-326: Optimize slug validation for performance.The current implementation loads all items and creates a HashSet for validation. For very large inventories, this could be memory-intensive.
Consider implementing a more targeted approach:
- When validating a single slug, possibly use a direct API call first
- Only fall back to the full collection if needed
- Consider Bloom filters for high-performance approximate membership testing
The current implementation is acceptable for typical collection sizes but might need optimization if the number of items grows significantly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)src/market/client.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/riven.rs(1 hunks)src/market/models/riven_group.rs(1 hunks)src/market/models/riven_type.rs(1 hunks)src/market/models/versions.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/market/models/riven.rs (1)
src/market/models/versions.rs (1)
serde_json(53-53)
src/market/models/mod.rs (3)
src/market/client.rs (2)
response(246-246)response(274-274)src/market/models/riven.rs (1)
riven(45-53)src/worldstate/models/base.rs (1)
query(76-87)
src/market/client.rs (2)
src/market/cache.rs (1)
new(16-21)src/market/models/mod.rs (1)
query(34-53)
🪛 LanguageTool
README.md
[style] ~5-~5: Consider using a more polite wording.
Context: ...0db1a09932459d). Use this crate if you want to make a Warframe-related rust project...
(IF_YOU_WANT)
🔇 Additional comments (25)
README.md (2)
10-11: MSRV Documentation Added
Including the note on the Minimum Supported Rust Version (1.85) is a helpful update for users. This clarifies the compatibility requirements right up front.
14-29: Example Code Block Alignment
The modifications in the example code block are well executed:
- The import statement now explicitly brings in
Client,Error,queryable::Cetus,Opposite, andTimedEvent, which enhances clarity compared to using a broader prelude.- The
mainfunction signature has been updated to returnResult<(), Error>, aligning with the new error type used throughout the project.- The async example correctly utilizes the
?operator for error propagation, keeping the code idiomatic and concise.Overall, these changes ensure that the documentation and code examples remain consistent with the module restructuring and updated error handling.
src/market/models/riven_group.rs (1)
3-13: Well-designed enum for riven weapon groupsThe
RivenGroupenum correctly implements all necessary traits for serialization, comparison, and use in collections. The lowercase serialization attribute aligns well with common API conventions.src/market/models/riven_type.rs (1)
3-12: Properly structured enum for riven typesThe
RivenTypeenum correctly implements all necessary traits and maintains consistency with theRivenGroupenum. The lowercase serialization ensures compatibility with the API.src/market/models/versions.rs (2)
5-14: Well-structured implementation for versions endpointThe
Versionsstruct is correctly mapped to the API endpoint using theimpl_queryable!macro and has appropriate fields with correct types for the version information.
42-56: Appropriate test setup using rstestThe test correctly uses rstest with file fixtures for testing JSON deserialization, which is a maintainable approach for testing model compatibility with API responses.
src/market/models/riven.rs (3)
12-25: Well-designed struct for riven weapons endpointThe
Rivenstruct is properly documented and structured with appropriate fields and types, including the optionalRivenGroupandRivenTypefields. The clippy suppression is justified for domain-specific terminology.
36-53: Good test setup using fixturesThe test correctly uses rstest with a JSON fixture for testing deserialization, which is an efficient approach to ensure API compatibility. The test follows the same pattern as other model tests in the codebase.
27-34:Details
❓ Verification inconclusive
Inconsistent field visibility in RivenI18N struct
The
item_nameandwiki_linkfields are private, whileiconandthumbare public. This inconsistency in visibility might cause issues if external code needs to access the private fields.Consider making all fields public for consistency:
#[derive(Debug, Deserialize, PartialEq, Clone)] #[serde(rename_all = "camelCase")] pub struct RivenI18N { - item_name: Option<String>, - wiki_link: Option<String>, + pub item_name: Option<String>, + pub wiki_link: Option<String>, pub icon: String, pub thumb: String, }Let's verify if these fields are accessed outside this module:
🏁 Script executed:
#!/bin/bash # Search for any usage of RivenI18N fields outside this file echo "Searching for external access to RivenI18N fields..." rg "\.item_name" --type rust --glob "!src/market/models/riven.rs" rg "\.wiki_link" --type rust --glob "!src/market/models/riven.rs" rg "\.icon" --type rust --glob "!src/market/models/riven.rs" | grep -i "riveni18n" rg "\.thumb" --type rust --glob "!src/market/models/riven.rs" | grep -i "riveni18n"Length of output: 417
Action: Verify Field Visibility Consistency in RivenI18N
In the
src/market/models/riven.rsfile (lines 27–34), theRivenI18Nstruct exhibits inconsistent field visibility—iconandthumbare public whileitem_nameandwiki_linkremain private. Although our automated search for external accesses ofitem_nameandwiki_linkproduced no results, this outcome is inconclusive. Please manually verify whether the private fields are meant to be accessed by external code. If they are part of the public API, consider updating the struct for consistency:#[derive(Debug, Deserialize, PartialEq, Clone)] #[serde(rename_all = "camelCase")] pub struct RivenI18N { - item_name: Option<String>, - wiki_link: Option<String>, + pub item_name: Option<String>, + pub wiki_link: Option<String>, pub icon: String, pub thumb: String, }Alternatively, if the design intentionally restricts direct access to
item_nameandwiki_link(perhaps exposing them through getter methods), then you may also want to reconsider whethericonandthumbshould be public or if the API should be adjusted for consistency.Please ensure that the intended usage of
RivenI18Nis preserved and update the code accordingly.src/market/mod.rs (7)
1-3: Well-structured module documentation.The documentation has been effectively improved with a clear purpose statement and a link to the API documentation, which enhances usability.
4-5: Good feature-gating for cache module.Properly using feature flags for optional functionality helps keep the crate lightweight for users who don't need caching capabilities.
8-8: Appropriate visibility restriction for models module.Changing from
pubtopub(crate)restricts the models module to crate-level visibility, which is a good encapsulation practice. This prevents users from depending directly on implementation details.
11-20: Clean organization of re-exports.The use of organized re-exports with appropriate grouping makes the module's public API clear and well-structured.
46-52: Well-designed queryable module.The queryable module provides a focused public API by re-exporting only the specific models that should be queryable by users, which is a good design practice.
56-97: Well-implemented Slug type with comprehensive trait support.The Slug type is well-designed with:
- Appropriate trait derivations
- Clear documentation
- Comprehensive conversion implementations
- Proper equality comparisons with different string types
This makes the type ergonomic to use throughout the codebase.
99-109: Good test coverage for Slug functionality.The tests thoroughly verify the snake_case conversion logic for different input patterns, which is essential for ensuring the Slug type works correctly.
src/market/models/mod.rs (3)
21-27: Well-structured ResponseBase type.The ResponseBase struct is properly designed with:
- Appropriate serde attributes for camelCase JSON
- Generic type parameter for flexible response data
- Optional fields for handling different API response scenarios
This provides a solid foundation for handling API responses.
29-54: Effective Queryable trait implementation.The Queryable trait is well-designed with:
- Appropriate associated types and constraints
- A comprehensive query method with proper error handling
- The
#[must_use]attribute to prevent ignored resultsThis provides a consistent pattern for API queries across different model types.
56-69: Good use of macro for reducing boilerplate.The
impl_queryablemacro efficiently handles the two common patterns for implementing the Queryable trait, reducing code duplication and potential errors.src/market/client.rs (6)
45-86: Well-structured Client with Builder pattern.The Client struct is well-designed with:
- Proper use of derive_builder for flexible instantiation
- Well-organized fields with appropriate defaults
- Feature-gated optional components
- Sensible cache TTL values for different data types
The builder pattern greatly improves the API's usability.
106-122: Well-implemented cache retrieval with proper error handling.The
get_from_cachemethod properly handles type safety withdowncast_refand includes appropriate tracing for debugging cache hits.
145-161: Good API design with convenience methods.The
fetchandfetch_itemmethods provide convenient defaults (using English language) while allowing more specific calls through theirfetch_using_languageandfetch_item_using_languagecounterparts. This API design is user-friendly.
167-187: Good implementation of caching in fetch_using_language.The method follows a proper pattern:
- Check cache first
- Apply rate limiting
- Fetch from API if needed
- Cache result for future use
This approach minimizes API calls while still providing fresh data.
226-256: Clear error handling in try_get_item.The method properly handles:
- Item not found cases (StatusCode::NOT_FOUND)
- Successful responses that need to be cached
- API error responses
The use of
Option<T>to represent missing items is appropriate.
336-343: Good use of macro for rate limiting.The
ratelimit!macro elegantly handles conditional compilation and provides a clean way to apply rate limiting throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/market/models/mod.rs (1)
20-29:⚠️ Potential issueMissing Future import.
The code uses
Futurein the trait definition on line 47 but doesn't import it. This might lead to compilation errors.use std::fmt::Debug; use i18n::Language; use serde::{ Deserialize, de::DeserializeOwned, }; +use std::future::Future; use super::BASE_URL; use crate::market::error::Error;
🧹 Nitpick comments (9)
src/market/models/riven_type.rs (1)
1-13: Looks good! Consider adding documentation.The
RivenTypeenum implementation is clean and follows Rust best practices with appropriate trait derivations. The#[serde(rename_all = "lowercase")]ensures proper JSON serialization compatibility.Consider adding documentation comments (using
///) to explain what riven types represent in the game context:use derive_more::Display; use serde::Deserialize; +/// Represents the different categories of Riven mods in the game. +/// Each type is associated with a specific weapon category. #[derive(Debug, Deserialize, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Hash, Display)] #[serde(rename_all = "lowercase")] pub enum RivenType { + /// Riven mods for secondary weapons Pistol, + /// Riven mods for melee weapons Melee, + /// Riven mods for primary rifle-type weapons Rifle, + /// Riven mods for shotgun-type weapons Shotgun, + /// Riven mods for custom melee Zaw weapons Zaw, + /// Riven mods for custom secondary Kitgun weapons Kitgun, }src/market/models/fixtures/lich_ephemera.json (1)
1-104: JSON fixture looks well-structured.The ephemera fixture contains a comprehensive list of in-game "Vengeful" ephemeras with appropriate metadata and internationalization support.
The fixture currently only includes English localization. If the application supports multiple languages, consider adding other language entries to test internationalization.
src/market/models/sister_quirk.rs (1)
37-46: Test function name doesn't match the struct being testedThe test function is named
lich_quirkbut it's testingSisterQuirkdeserialization. This inconsistency could lead to confusion.- fn lich_quirk( + fn sister_quirk(Additionally, the test is using a fixture file named "lich_quirk.json" to test
SisterQuirk. Consider creating a dedicated fixture file for sister quirks if the data structure differs from lich quirks.src/market/models/sister_ephemera.rs (1)
38-42: Test name and fixture path inconsistency.The test function is named
lich_ephemerabut it's testing theSisterEphemerastruct. Additionally, it's using a fixture file path for lich ephemera. This could lead to confusion or maintenance issues.Consider renaming the test to match the struct being tested:
- fn lich_ephemera( + fn sister_ephemera( #[files("src/market/models/fixtures/lich_ephemera.json")] #[mode = str] json: &str,If the sister and lich ephemera use the same data structure but different endpoints, please add a comment explaining why they share the same test fixture.
src/market/models/sister_weapon.rs (1)
38-42: Test name and fixture path inconsistency.The test function is named
lich_weaponbut it's testing theSisterWeaponstruct. Additionally, it's using a fixture file path for lich weapons. This could lead to confusion or maintenance issues.Consider renaming the test to match the struct being tested:
- fn lich_weapon( + fn sister_weapon( #[files("src/market/models/fixtures/lich_weapon.json")] #[mode = str] json: &str,If sister and lich weapons share a similar structure and can use the same test fixture, please add a comment explaining this relationship.
src/market/models/mod.rs (1)
39-64: Well-designed Queryable trait with effective error handling.The
Queryabletrait provides a clean abstraction for API queries with good error handling. The implementation efficiently handles API errors and empty responses.One potential improvement: consider adding a timeout parameter or configuration to prevent hanging on slow API responses.
src/market/client.rs (3)
113-117: Avoid panicking in library code.
Whileexpect("default client builder should never fail")is rarely triggered, returning aResultis often safer for library code.-pub fn new() -> Self { - ClientBuilder::default() - .build() - .expect("default client builder should never fail") +pub fn new() -> Result<Self> { + ClientBuilder::default() + .build() + .map_err(|err| Error::Initialization(err.to_string())) }
258-300: Consider a specialized typed cache.
Storing multiple data types under the sameArc<dyn Any>cache can complicate usage and debugging. Introducing a dedicated typed cache (e.g., one per entity type) could streamline downcasting and improve clarity.
396-401: Validate naming for theratelimit!macro.
Given thatuse ratelimit;also appears, consider renaming the macro or the module import to avoid potential identifier conflicts or confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/market/cache.rs(1 hunks)src/market/client.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/fixtures/lich_ephemera.json(1 hunks)src/market/models/fixtures/lich_quirk.json(1 hunks)src/market/models/fixtures/lich_weapon.json(1 hunks)src/market/models/fixtures/mission.json(1 hunks)src/market/models/fixtures/riven_attribute.json(1 hunks)src/market/models/lich_ephemera.rs(1 hunks)src/market/models/lich_quirk.rs(1 hunks)src/market/models/lich_weapon.rs(1 hunks)src/market/models/location.rs(1 hunks)src/market/models/mission.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/npc.rs(1 hunks)src/market/models/riven_attribute.rs(1 hunks)src/market/models/riven_group.rs(1 hunks)src/market/models/riven_type.rs(1 hunks)src/market/models/sister_ephemera.rs(1 hunks)src/market/models/sister_quirk.rs(1 hunks)src/market/models/sister_weapon.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/market/models/fixtures/lich_weapon.json
- src/market/models/fixtures/mission.json
- src/market/models/fixtures/riven_attribute.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/market/models/riven_group.rs
🧰 Additional context used
🧬 Code Definitions (10)
src/market/models/lich_quirk.rs (9)
src/market/models/sister_quirk.rs (2)
lich_quirk(38-46)serde_json(43-43)src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/lich_weapon.rs (1)
serde_json(44-44)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_weapon.rs (1)
serde_json(44-44)
src/market/models/lich_weapon.rs (9)
src/market/models/sister_weapon.rs (2)
lich_weapon(39-47)serde_json(44-44)src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)
src/market/models/location.rs (9)
src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/lich_weapon.rs (1)
serde_json(44-44)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)src/market/models/sister_weapon.rs (1)
serde_json(44-44)
src/market/models/riven_attribute.rs (9)
src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/lich_weapon.rs (1)
serde_json(44-44)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)src/market/models/sister_weapon.rs (1)
serde_json(44-44)
src/market/models/npc.rs (9)
src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/lich_weapon.rs (1)
serde_json(44-44)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)src/market/models/sister_weapon.rs (1)
serde_json(44-44)
src/market/mod.rs (13)
src/market/models/mod.rs (1)
client(49-54)src/market/models/lich_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/sister_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/lich_quirk.rs (1)
lich_quirk(38-46)src/market/models/sister_quirk.rs (1)
lich_quirk(38-46)src/market/models/lich_weapon.rs (1)
lich_weapon(39-47)src/market/models/sister_weapon.rs (1)
lich_weapon(39-47)src/market/models/location.rs (1)
location(67-75)src/market/models/mission.rs (1)
mission(37-45)src/market/models/npc.rs (1)
npc(37-45)src/market/models/riven.rs (1)
riven(45-53)src/market/models/riven_attribute.rs (1)
riven_attribute(77-85)src/market/cache.rs (1)
new(21-26)
src/market/models/mission.rs (9)
src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/lich_weapon.rs (1)
serde_json(44-44)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)src/market/models/sister_weapon.rs (1)
serde_json(44-44)
src/market/models/sister_weapon.rs (9)
src/market/models/lich_weapon.rs (2)
lich_weapon(39-47)serde_json(44-44)src/market/models/lich_ephemera.rs (1)
serde_json(44-44)src/market/models/lich_quirk.rs (1)
serde_json(43-43)src/market/models/location.rs (1)
serde_json(72-72)src/market/models/mission.rs (1)
serde_json(42-42)src/market/models/npc.rs (1)
serde_json(42-42)src/market/models/riven_attribute.rs (1)
serde_json(82-82)src/market/models/sister_ephemera.rs (1)
serde_json(44-44)src/market/models/sister_quirk.rs (1)
serde_json(43-43)
src/market/client.rs (3)
src/market/cache.rs (1)
new(21-26)src/market/models/mod.rs (1)
query(44-63)src/worldstate/models/mod.rs (1)
query(93-95)
src/market/models/mod.rs (12)
src/market/client.rs (2)
response(283-283)response(318-318)src/market/models/lich_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/sister_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/lich_quirk.rs (1)
lich_quirk(38-46)src/market/models/sister_quirk.rs (1)
lich_quirk(38-46)src/market/models/lich_weapon.rs (1)
lich_weapon(39-47)src/market/models/sister_weapon.rs (1)
lich_weapon(39-47)src/market/models/location.rs (1)
location(67-75)src/market/models/mission.rs (1)
mission(37-45)src/market/models/npc.rs (1)
npc(37-45)src/market/models/riven.rs (1)
riven(45-53)src/market/models/riven_attribute.rs (1)
riven_attribute(77-85)
🔇 Additional comments (28)
src/market/models/lich_weapon.rs (4)
8-8: Macro implementation correctly sets up the API endpoint mapping.The
impl_queryable!macro associates theLichWeaponstruct with the "/lich/weapons" endpoint, which follows the pattern used in other model implementations.
10-19: Well-structured model with appropriate annotations.The
LichWeaponstruct is properly defined with the necessary fields and derives for serialization/deserialization. The camelCase attribute ensures compatibility with the API response format.
21-28: I18N struct follows consistent pattern.The
LichWeaponI18Nstruct is correctly implemented with the necessary fields for localization and proper serialization attributes.
30-48:Details
✅ Verification successful
Test implementation is consistent with other models.
The test module follows the established pattern of using
rstestto validate deserialization from fixture data, which is good practice for ensuring data model compatibility.To ensure consistency with other model tests, run:
🏁 Script executed:
#!/bin/bash # Check if test patterns are consistent across model tests # Check how many files follow similar test pattern rg -A 3 "fn .*_weapon\(" src/market/models/Length of output: 572
Consistent Test Implementation
The test in
src/market/models/lich_weapon.rsadheres to the established pattern used in similar model tests (e.g., insrc/market/models/sister_weapon.rs). It correctly usesrstestand fixture-based deserialization to ensure data model compatibility.src/market/models/lich_quirk.rs (3)
1-18: Well-structured model implementation for Lich QuirksThe
LichQuirkstruct is properly defined with all necessary fields and correctly implementsDeserializefor JSON parsing. The use ofimpl_queryable!macro to associate the struct with the "/lich/quirks" endpoint follows the established pattern in other model files.
20-27: Appropriate internationalization structureThe
LichQuirkI18Nstruct correctly defines fields for internationalized content with appropriate optionality for fields that might not always be present in the API response.
29-46: Good test coverage using fixture-based testingThe test module effectively uses rstest to verify JSON deserialization from fixtures, which is a good practice for ensuring API compatibility without making actual network requests.
src/market/models/mission.rs (3)
1-18: Clean implementation of Mission modelThe
Missionstruct follows the same pattern as other models in the codebase with proper documentation and serialization attributes.
20-26: Appropriate internationalization structure for MissionThe
MissionI18Nstruct properly handles internationalized content with appropriate optionality for fields that might not always be present.
28-45: Effective test implementationThe test module correctly uses rstest to verify JSON deserialization from fixtures, maintaining consistency with other model tests.
src/market/cache.rs (4)
1-9: Good use of standard library componentsThe cache implementation correctly uses
HashSetfor efficient lookups andArcfor thread-safe shared ownership of slugs.
10-18: Well-designed CacheKey with appropriate traitsThe
CacheKeystruct derives all the necessary traits for use in maps and collections (Hash,PartialEq,Eq, etc.). The documentation clearly explains the purpose of theendpointfield.
20-27: Clean constructor implementationThe
newmethod provides a convenient way to createCacheKeyinstances, converting the endpoint string to anArc<str>for efficient memory usage.
29-40: Comprehensive SlugContext enumThe
SlugContextenum covers all necessary categories for the different types of slugs, following Rust's idiomatic approach to categorization with enums.src/market/models/sister_quirk.rs (2)
1-18: Well-structured model implementation for Sister QuirksThe
SisterQuirkstruct follows the same pattern as other models with proper documentation and serialization attributes.
20-27: Appropriate internationalization structureThe
SisterQuirkI18Nstruct correctly defines fields for internationalized content with appropriate optionality.src/market/models/lich_ephemera.rs (1)
8-20: LGTM! Implementation looks clean and well-structured.The struct definitions and endpoint association are properly implemented. The struct includes all necessary fields for representing the lich ephemera data from the API.
src/market/models/npc.rs (1)
1-46: LGTM! Well-structured implementation with proper test naming.The
NpcandNpcI18Nstructs are well-defined with appropriate serialization attributes. The test function is correctly named to match the struct being tested and uses a corresponding fixture file.src/market/models/location.rs (1)
1-76: Well-structured model implementation with good testing approach.The
Locationmodule is well-organized with proper serialization/deserialization handling, clear documentation, and appropriate use of types. The implementation of internationalization through theI18Ngeneric struct is a good design choice. The test fixture approach usingrstestis also a solid testing strategy.src/market/models/riven_attribute.rs (1)
1-86: Clean implementation with comprehensive type definitions.The
RivenAttributemodule is well-structured with appropriate field types and defaults. I particularly like the detailed documentation on theUnitenum variants, which provides clear context for each option. The comprehensive derive macros for the enum also ensure it can be used in various contexts (comparison, hashing, display, etc.).src/market/mod.rs (2)
86-125: Well-designed Slug utility with comprehensive implementations.The
Slugtype is a well-designed utility for converting user-friendly strings into URL-friendly slugs. The implementation includes appropriate trait derivations and conversions from various string types. The equality implementations are particularly useful for comparing slugs with string values directly.
64-80: Good organization of queryable models through re-exports.The dedicated
queryablemodule that re-exports specific models with querying capabilities improves code organization and makes the public API more discoverable.src/market/models/mod.rs (1)
66-79: Efficient macro for reducing boilerplate.The
impl_queryablemacro effectively reduces boilerplate by standardizing the implementation of theQueryabletrait for different types. The distinction between Array and Object types is a good design decision.src/market/client.rs (5)
61-63: Encapsulate builder-specific configuration.
Usingderive_builderand setting#[builder(pattern = "owned")]is a clean way to construct complex structs. This approach enhances code readability and maintainability.
142-149: Appreciation for cache insertion approach.
Inserting data into the Moka cache viaArcis a straightforward and thread-safe pattern.
336-366: Efficient slug retrieval.
Using the cache to store slug sets for eachSlugContextis a clean approach. It avoids repetitive API calls and improves performance.
405-415: Commend theto_hashset!macro usage.
Wrapping the enumeration of fetch results into a neat macro is an elegant solution, reducing boilerplate for buildingHashSets.
124-140:Details
✅ Verification successful
Check for consistent caching logic.
Theget_from_cachefunction usesArc<dyn Any + Send + Sync>and downcasts toT. Ensure that all consumers properly handle possible mismatches or downcast failures.Please verify each call site of
get_from_cacheto confirm that the stored type always matches the requested type:
🏁 Script executed:
#!/bin/bash # Searching for get_from_cache calls and checking the type usage rg -A 5 'get_from_cache'Length of output: 950
Consistent Caching Logic Verified
All identified call sites for
get_from_cache(using both<T>and<T::Data>) properly check for a successful downcast usingif let Some(...). This confirms that any type mismatch or downcast failure is gracefully handled by returningNone, and no unsafe assumptions are made about the stored type.No further action is required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/market/models/mod.rs (1)
43-68: Missing Future import.The code uses
Futurein the trait definition but doesn't import it. This might lead to compilation errors.use std::fmt::Debug; use i18n::Language; use serde::{ Deserialize, de::DeserializeOwned, }; +use std::future::Future; use super::BASE_URL; use crate::market::error::Error;src/market/client.rs (1)
117-120: Still panicking during construction – propagate the error instead
Client::new()now usesexpect, but that still panics in library code.
Returning aResult<Self, reqwest::Error>(or your domainError) gives consumers a chance to handle mis‑configuration gracefully.This is the same concern raised in an earlier review; please consider adapting it.
🧹 Nitpick comments (6)
src/market/models/order_with_user.rs (1)
33-42: Rename the test function to match its purpose.The test function is named "riven" but it's testing the
OrderWithUserstruct deserialization. This could be confusing, especially as there's a separaterivenmodule in the codebase.- fn riven( + fn test_order_with_user_deserialization( #[files("src/market/models/fixtures/orders.json")] #[mode = str] json: &str, ) -> Result<(), serde_json::Error> { serde_json::from_str::<ResponseBase<<OrderWithUser as Queryable>::Data>>(json)?; Ok(()) }src/market/models/user_short.rs (1)
26-26: Consider using a more specific type for timestamps.The
last_seenfield is using aStringtype, which works but doesn't provide any validation or utility methods for date/time operations. If the API returns a standardized timestamp format, consider using a more specific type likechrono::DateTime<chrono::Utc>.use serde::Deserialize; +use chrono::{DateTime, Utc}; use super::activity::Activity; #[derive(Debug, Clone, Deserialize, PartialEq, Eq, PartialOrd, Ord, Hash)] #[serde(rename_all = "camelCase")] pub struct UserShort { // ...other fields... /// Timestamp of the user's last online presence. - pub last_seen: String, + pub last_seen: DateTime<Utc>, }src/market/mod.rs (1)
119-135: Consider implementing symmetric equality forSlug.The current
PartialEqimplementations are asymmetric:
slug == stringworks- But
string == slugdoesn't workThis asymmetry could lead to unexpected behavior in comparisons. Consider implementing the reverse direction as well.
// Existing implementations // Add symmetric implementations +impl PartialEq<Slug> for String { + fn eq(&self, other: &Slug) -> bool { + self == &other.0 + } +} + +impl PartialEq<Slug> for str { + fn eq(&self, other: &Slug) -> bool { + self == other.0 + } +} + +impl PartialEq<Slug> for &str { + fn eq(&self, other: &Slug) -> bool { + *self == other.0 + } +}src/market/models/mod.rs (1)
70-83: Consider adding documentation to the macro.The
impl_queryable!macro is powerful but lacks documentation explaining its purpose and usage patterns.+/// A macro that implements the [`Queryable`] trait for a given type. +/// +/// # Parameters +/// - `$name`: The name of the type to implement the trait for. +/// - `Array` or `Object`: The response format, either an array of items or a single object. +/// - `$endpoint`: The API endpoint as a string literal. +/// +/// # Examples +/// ``` +/// impl_queryable!(OrderWithUser, Array, "/orders/recent"); +/// impl_queryable!(Versions, Object, "/versions"); +/// ``` macro_rules! impl_queryable { ($name:ident,Array, $endpoint:literal) => { impl crate::market::Queryable for $name { const ENDPOINT: &str = $endpoint; type Data = Vec<Self>; } }; ($name:ident,Object, $endpoint:literal) => { impl crate::market::Queryable for $name { const ENDPOINT: &str = $endpoint; type Data = Self; } }; }src/market/client.rs (2)
154-165: Surface HTTP‑level failures explicitly
fetch_from_apireturns the rawResponseeven if the status is 4xx/5xx (except 404 handled later).
Calling sites then attempt JSON deserialization, which will fail with a less helpful error.A small improvement:
- self.client - .get(format!("{BASE_URL}{endpoint}")) - .header("Language", language.to_string()) - .send() - .await + self.client + .get(format!("{BASE_URL}{endpoint}")) + .header("Language", language.to_string()) + .send() + .await? + .error_for_status() // converts HTTP errors into `reqwest::Error`This keeps API and transport errors separate and avoids unnecessary JSON parsing attempts.
420-435:use ratelimit;/use to_hashset;are no longer needed after macro re‑orderingOnce the macros are moved before first use, they are automatically in scope.
The bareusestatements can be removed to unclutter the import section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/market/client.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/activity.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/order.rs(1 hunks)src/market/models/order_with_user.rs(1 hunks)src/market/models/user_short.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/market/models/activity.rs
- src/market/models/order.rs
🔇 Additional comments (1)
src/market/models/mod.rs (1)
35-41: LGTM! Well-structured response type.The
ResponseBase<T>struct is well-designed for standardizing API response handling with appropriate fields for version, data, and error information.
BREAKING CHANGE: changed items api
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/market/models/item.rs (2)
57-66: Enhance test coverage to verify field values.The test only verifies that deserialization doesn't fail. Consider asserting expected values for key fields to ensure data correctness.
#[rstest::rstest] fn test_item( #[files("src/market/models/fixtures/item.json")] #[mode = str] json: &str, ) -> Result<(), serde_json::Error> { - serde_json::from_str::<ResponseBase<Item>>(json)?; + let response: ResponseBase<Item> = serde_json::from_str(json)?; + + // Verify the response structure + assert!(response.error.is_none()); + assert!(!response.data.is_empty()); + + // Verify first item's key fields + let first_item = &response.data[0]; + assert!(!first_item.id.is_empty()); + assert!(!first_item.slug.is_empty()); + assert!(first_item.i18n.en.is_some()); Ok(()) }
5-38: Consider the boolean field refactoring suggestion from previous review.While the
#[allow(clippy::struct_excessive_bools)]attribute suppresses the warning, the struct still has 6 boolean/optional boolean fields. This is a known issue from the previous review.src/market/models/mod.rs (1)
45-70: Missing Future import causes compilation errors.The
Queryabletrait usesFuturein its return type but doesn't import it, causing compilation failures.+use std::future::Future;Cargo.toml (1)
4-4: Verify Rust edition availability.Edition "2024" may not be officially released yet. This should be verified and potentially changed to "2021" until the 2024 edition is stable.
Is Rust edition 2024 officially released and stable for production use?src/market/client.rs (2)
400-413: Cache key may not include query parameters.The cache key is generated before query parameters are applied, potentially causing cache collisions for requests with different query strings to the same base endpoint.
The cache key should include the complete URL with query parameters to ensure uniqueness.
545-567: Macro definitions must precede their usage.The
ratelimit!andto_hashset!macros are defined at the bottom but used throughout the file. This causes compilation errors since macros must be defined before use.Move the macro definitions to the top of the file after the imports.
🧹 Nitpick comments (3)
src/profile/models/profile.rs (2)
103-129: Consider organizing daily affiliation fields.While the current approach with individual fields is correct for API mapping, the large number of daily affiliation counters could potentially be organized into a nested struct or HashMap for better maintainability.
+#[derive(Deserialize, Debug, Clone)] +pub struct DailyAffiliations { + pub daily_affiliation: u32, + pub daily_affiliation_pvp: u32, + pub daily_affiliation_library: u32, + pub daily_affiliation_cetus: u32, + pub daily_affiliation_quills: u32, + pub daily_affiliation_solaris: u32, + pub daily_affiliation_ventkids: u32, + pub daily_affiliation_vox: u32, + pub daily_affiliation_entrati: u32, + pub daily_affiliation_necraloid: u32, + pub daily_affiliation_zariman: u32, + pub daily_affiliation_kahl: u32, + pub daily_affiliation_cavia: u32, + pub daily_affiliation_hex: u32, +}Then flatten it in ProfileInfo:
+ #[serde(flatten)] + pub daily_affiliations: DailyAffiliations,
170-175: Simplify ObjectId deserialization.The
deserialize_oidfunction can be more direct instead of callingdeserialize_oid_or_noneand then converting None to an error.-pub(crate) fn deserialize_oid<'de, D>(deserializer: D) -> Result<String, D::Error> -where - D: Deserializer<'de>, -{ - deserialize_oid_or_none(deserializer)?.ok_or_else(|| de::Error::custom("missing $oid field")) -} +pub(crate) fn deserialize_oid<'de, D>(deserializer: D) -> Result<String, D::Error> +where + D: Deserializer<'de>, +{ + let v: Value = Deserialize::deserialize(deserializer)?; + v.get("$oid") + .and_then(Value::as_str) + .map(ToOwned::to_owned) + .ok_or_else(|| de::Error::custom("missing $oid field")) +}src/market/models/fixtures/mission.json (1)
1061-1155: Replace placeholder icons with actual mission icons.Several endless tier missions and Entrati Lab bounties are using placeholder icons (
missions/unknown.png). These should be updated with the actual mission icons when available.Would you like me to create an issue to track the missing mission icons for these entries?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)README.md(1 hunks)devtools.nu(1 hunks)rustfmt.toml(1 hunks)src/lib.rs(1 hunks)src/market/cache.rs(1 hunks)src/market/client.rs(1 hunks)src/market/error.rs(1 hunks)src/market/mod.rs(1 hunks)src/market/models/activity.rs(1 hunks)src/market/models/fixtures/item.json(1 hunks)src/market/models/fixtures/lich_ephemera.json(1 hunks)src/market/models/fixtures/lich_quirk.json(1 hunks)src/market/models/fixtures/lich_weapon.json(1 hunks)src/market/models/fixtures/mission.json(1 hunks)src/market/models/fixtures/riven_attribute.json(1 hunks)src/market/models/fixtures/top_orders.json(1 hunks)src/market/models/fixtures/versions.json(1 hunks)src/market/models/i18n.rs(1 hunks)src/market/models/item.rs(1 hunks)src/market/models/item_info.rs(0 hunks)src/market/models/item_short.rs(1 hunks)src/market/models/lich_ephemera.rs(1 hunks)src/market/models/lich_quirk.rs(1 hunks)src/market/models/lich_weapon.rs(1 hunks)src/market/models/location.rs(1 hunks)src/market/models/mission.rs(1 hunks)src/market/models/mod.rs(1 hunks)src/market/models/npc.rs(1 hunks)src/market/models/order.rs(1 hunks)src/market/models/order_with_user.rs(1 hunks)src/market/models/orders.rs(0 hunks)src/market/models/riven.rs(1 hunks)src/market/models/riven_attribute.rs(1 hunks)src/market/models/riven_group.rs(1 hunks)src/market/models/riven_type.rs(1 hunks)src/market/models/set_items.rs(1 hunks)src/market/models/sister_ephemera.rs(1 hunks)src/market/models/sister_quirk.rs(1 hunks)src/market/models/sister_weapon.rs(1 hunks)src/market/models/statistic_item.rs(0 hunks)src/market/models/top_orders.rs(1 hunks)src/market/models/top_orders_query_params.rs(1 hunks)src/market/models/user_short.rs(1 hunks)src/market/models/versions.rs(1 hunks)src/profile/client.rs(1 hunks)src/profile/error.rs(1 hunks)src/profile/mod.rs(1 hunks)src/profile/models/affiliation.rs(1 hunks)src/profile/models/color_load_out.rs(1 hunks)src/profile/models/fixtures/profile_payload.json(1 hunks)src/profile/models/focus_school.rs(1 hunks)src/profile/models/guild_tier.rs(1 hunks)src/profile/models/load_out_inventory.rs(1 hunks)src/profile/models/load_out_preset.rs(1 hunks)src/profile/models/mod.rs(1 hunks)src/profile/models/operator_load_out.rs(1 hunks)src/profile/models/platform.rs(1 hunks)src/profile/models/player_skill.rs(1 hunks)src/profile/models/profile.rs(1 hunks)src/profile/models/stats.rs(1 hunks)src/worldstate/client.rs(13 hunks)src/worldstate/fixtures/deep_archimedea.rs(1 hunks)src/worldstate/fixtures/mod.rs(1 hunks)src/worldstate/mod.rs(1 hunks)src/worldstate/models/base.rs(6 hunks)src/worldstate/models/cetus.rs(1 hunks)src/worldstate/models/damage_type.rs(3 hunks)src/worldstate/models/deep_archimedea.rs(1 hunks)
💤 Files with no reviewable changes (3)
- src/market/models/item_info.rs
- src/market/models/orders.rs
- src/market/models/statistic_item.rs
✅ Files skipped from review due to trivial changes (1)
- src/worldstate/fixtures/deep_archimedea.rs
🚧 Files skipped from review as they are similar to previous changes (56)
- .gitignore
- .github/workflows/release.yml
- src/profile/mod.rs
- src/market/models/set_items.rs
- src/market/models/riven_group.rs
- rustfmt.toml
- src/market/models/riven_type.rs
- src/profile/models/guild_tier.rs
- src/lib.rs
- src/market/models/fixtures/versions.json
- src/market/models/fixtures/lich_ephemera.json
- src/profile/models/mod.rs
- src/market/models/i18n.rs
- src/market/models/order_with_user.rs
- src/market/models/mission.rs
- src/market/models/fixtures/lich_quirk.json
- src/profile/error.rs
- src/market/models/activity.rs
- src/worldstate/models/cetus.rs
- src/profile/client.rs
- src/market/models/lich_weapon.rs
- src/profile/models/operator_load_out.rs
- src/market/models/sister_ephemera.rs
- src/market/models/lich_quirk.rs
- src/market/models/fixtures/top_orders.json
- src/market/models/location.rs
- src/market/models/order.rs
- src/profile/models/platform.rs
- src/market/models/item_short.rs
- src/market/models/riven.rs
- src/market/models/versions.rs
- src/profile/models/player_skill.rs
- src/market/models/top_orders.rs
- src/profile/models/focus_school.rs
- devtools.nu
- src/worldstate/models/damage_type.rs
- src/market/models/top_orders_query_params.rs
- src/market/models/user_short.rs
- src/market/models/sister_quirk.rs
- src/worldstate/models/deep_archimedea.rs
- src/market/models/riven_attribute.rs
- README.md
- src/profile/models/stats.rs
- src/market/error.rs
- src/market/models/lich_ephemera.rs
- src/market/models/sister_weapon.rs
- src/profile/models/color_load_out.rs
- src/market/models/fixtures/item.json
- src/market/models/npc.rs
- src/profile/models/fixtures/profile_payload.json
- src/profile/models/load_out_inventory.rs
- src/profile/models/affiliation.rs
- src/profile/models/load_out_preset.rs
- src/worldstate/models/base.rs
- src/worldstate/client.rs
- src/worldstate/fixtures/mod.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#19
File: src/market/client.rs:184-185
Timestamp: 2024-10-21T15:24:20.159Z
Learning: In the Rust codebase for 'warframe.rs', when a code path is expected to be unreachable, prefer using `unreachable!()` macro instead of `panic!()`.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
src/market/cache.rs (1)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
src/market/mod.rs (1)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
src/profile/models/profile.rs (1)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
src/market/models/mod.rs (2)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#19
File: src/market/client.rs:184-185
Timestamp: 2024-10-21T15:24:20.159Z
Learning: In the Rust codebase for 'warframe.rs', when a code path is expected to be unreachable, prefer using `unreachable!()` macro instead of `panic!()`.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
src/worldstate/mod.rs (2)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#20
File: src/worldstate/models/damage_type.rs:43-63
Timestamp: 2024-10-27T11:49:28.782Z
Learning: In `src/worldstate/models/damage_type.rs`, the `Void` and `Tau` elements do not form combined elements with other elements in the `combine` function.
Cargo.toml (3)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#19
File: src/market/client.rs:184-185
Timestamp: 2024-10-21T15:24:20.159Z
Learning: In the Rust codebase for 'warframe.rs', when a code path is expected to be unreachable, prefer using `unreachable!()` macro instead of `panic!()`.
src/market/client.rs (4)
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#19
File: src/market/client.rs:184-185
Timestamp: 2024-10-21T15:24:20.159Z
Learning: In the Rust codebase for 'warframe.rs', when a code path is expected to be unreachable, prefer using `unreachable!()` macro instead of `panic!()`.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Learnt from: Mettwasser
PR: WFCD/warframe.rs#24
File: src/market/client.rs:121-125
Timestamp: 2025-06-18T04:45:53.538Z
Learning: When using derive_builder with all fields having defaults (either through #[builder(default)] or explicit default values in #[builder(...)]), the generated builder's build() method cannot fail with UninitializedFieldError since no fields are left uninitialized. In such cases, using expect() with an appropriate message is safe and acceptable.
🧬 Code Graph Analysis (2)
src/profile/models/profile.rs (1)
src/profile/models/platform.rs (1)
deserialize(22-61)
src/market/models/mod.rs (14)
src/market/client.rs (3)
response(368-368)response(432-432)response(467-467)src/market/models/lich_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/sister_ephemera.rs (1)
lich_ephemera(39-47)src/market/models/top_orders.rs (1)
lich_quirk(17-25)src/market/models/lich_quirk.rs (1)
lich_quirk(38-46)src/market/models/sister_quirk.rs (1)
lich_quirk(38-46)src/market/models/lich_weapon.rs (1)
lich_weapon(39-47)src/market/models/sister_weapon.rs (1)
lich_weapon(39-47)src/market/models/location.rs (1)
location(67-75)src/market/models/mission.rs (1)
mission(37-45)src/market/models/npc.rs (1)
npc(37-45)src/market/models/order_with_user.rs (1)
riven(34-42)src/market/models/riven.rs (1)
riven(45-53)src/market/models/riven_attribute.rs (1)
riven_attribute(77-85)
🔇 Additional comments (14)
src/profile/models/profile.rs (4)
1-2: Appropriate use of lint allowance for data model.The
clippy::struct_excessive_boolsallowance is justified given the nature of profile data with numerous boolean flags.
25-40: Well-structured Profile root struct.The struct correctly uses serde attributes for JSON field mapping and handles optional fields appropriately.
186-198: Excellent MongoDB date deserialization implementation.The function correctly handles the nested MongoDB date format with proper error handling and clear error messages.
200-644: Outstanding comprehensive test coverage.The test suite is exceptionally thorough, covering all struct fields, nested objects, collections, and edge cases. This level of testing ensures reliable deserialization and provides excellent documentation of the expected data structure.
src/market/cache.rs (1)
1-41: LGTM!The cache implementation is well-structured with appropriate type definitions and trait derivations. The use of
Arc<str>for the endpoint field andArc<HashSet<String>>for slugs is consistent with the codebase's design principle of returning direct value types while managing shared ownership internally.src/market/models/fixtures/lich_weapon.json (1)
1-266: LGTM!The Kuva Lich weapon fixture is well-structured with consistent data format. All entries follow the expected schema with appropriate mastery rank requirements.
src/market/models/fixtures/riven_attribute.json (1)
710-712: Confirm empty prefix/suffix for the negative-only Riven attributeIt looks like the only entry with
"negativeOnly": truein
src/market/models/fixtures/riven_attribute.jsonhas bothprefixandsuffixset to empty strings, whereas all other attributes define non-empty values:• File: src/market/models/fixtures/riven_attribute.json
Lines ~710–712:"group": "melee", "prefix": "", "suffix": "",Please verify that omitting both prefix and suffix here is intentional (e.g. your UI layer applies “–” or “%” elsewhere) and matches product requirements.
src/market/mod.rs (1)
1-158: LGTM! Well-structured market module with comprehensive API surface.The market module refactoring effectively organizes the V2 API implementation with:
- Proper conditional compilation for cache features
- Clean separation of concerns with private client module
- Comprehensive re-exports providing good developer experience
- Well-implemented
Slugutility with robust test coverageThe
Slugimplementation correctly handles edge cases and provides intuitive conversion methods.src/worldstate/mod.rs (1)
1-114: Excellent refactoring from prelude to explicit queryable module.This migration improves the API design by:
- Making imports explicit and discoverable through the
queryablemodule- Providing clear separation between internal and external types
- Maintaining comprehensive re-exports for backward compatibility
- Including well-updated documentation examples
The new structure is more maintainable and provides better developer experience.
src/market/models/mod.rs (1)
26-88: Well-designed generic API structure withResponseBase<T>andQueryabletrait.The implementation provides:
- Clean generic response wrapper with camelCase serialization
- Unified trait interface for queryable endpoints
- Helpful macro for implementing the trait pattern
- Proper error handling with fallback mechanisms
The design enables consistent API interactions across different model types.
Cargo.toml (2)
49-51: Modern workspace configuration with proper dependency management.The workspace setup correctly includes the new
warframe-macroscrate and uses resolver "3" for better dependency resolution. The feature gates for caching and rate limiting are well-designed.
11-11: MSRV set to 1.85 is within the recommended range for 2025
- Current stable Rust: 1.88.0
- Typical library MSRV in 2025: 1.70.0–1.85.0
- Rust 2024 edition requires MSRV ≥ 1.85.0 (if you’re using its features)
You’re aligned with best practices. To maximize compatibility, verify your crate still builds on the lowest supported toolchains (e.g. via
rustuporcargo-msrv).src/market/client.rs (2)
69-125: Well-designed builder pattern with proper defaults.The client structure effectively uses
derive_builderwith:
- Conditional compilation for optional features
- Sensible defaults for all fields including caches and rate limiter
- Safe constructor that cannot fail due to complete defaults
The builder pattern provides flexibility while maintaining usability.
172-215: Generic fetch methods provide excellent abstraction.The
fetch<T>()andfetch_using_language<T>()methods create a clean, type-safe interface that:
- Leverages the
Queryabletrait for consistency- Transparently handles caching when enabled
- Integrates rate limiting seamlessly
- Provides proper error handling
This design significantly improves the API ergonomics.
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
4 participants
What did you fix?
Closes #21 and #23
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
New Features
ItemStringWrapperfor streamlined item querying.Refactor
Documentation
Chores
.imlto.gitignoreto exclude IDE files.