Initial implementation #2
Conversation
… derivitives, multiple fixes.
Make each module a default feature Change the IDs from strings to a generic type Change the error from the boxed dynamic to a generic type Change manual conversion functions to use `Into` instead
|
|
||
| #[derive(thiserror::Error, Debug)] | ||
| #[error("The trait is not implemented")] | ||
| pub struct UnimplementedError; |
There was a problem hiding this comment.
Wouldn't it be better if it was a trait that depends on std::error:Error?
So that we can fall back to more normal methods if the need arises, or if we need something like the step the error happened for example
| #[error("The trait is not implemented")] | ||
| pub struct UnimplementedError; | ||
|
|
||
| pub(crate) type Datetime = chrono::DateTime<chrono::Utc>; |
There was a problem hiding this comment.
UtcDateTime, it can be confused with the generic one it shadows otherwise
|
|
||
| async fn get_projects(&self, ids: Vec<&ID>) -> Result<Vec<Project<ID>>, E> { | ||
| let mut projects = Vec::new(); | ||
| for id in ids { |
There was a problem hiding this comment.
Not totally sure about that, but wouldn't this mean that we are working a project at a time?
Could something like https://docs.rs/futures/latest/futures/future/fn.join_all.html help?
There was a problem hiding this comment.
Sometimes too many concurrent requests completely fails, I had to deal with this problem when implementing concurrent requests, and it was a rather huge pain. I ended up using Semaphore which I don't think you can use with join_all(), and semaphore would need to import tokio which we certainly don't want.
There was a problem hiding this comment.
Seems like we have the first design problem lol
But well, it's not like it is a huge problem, and the solution (Having pending requests we can start at any time) is even worse
There was a problem hiding this comment.
Well the only platform that doesn't implement multiple requests is github (the others overrides this default implementation), and we won't be using the multiple requests on github afaik.
| async fn get_project_dependencies( | ||
| &self, | ||
| project_id: &ID, | ||
| ) -> Result<(Vec<Project<ID>>, Vec<Version<ID>>), E>; |
There was a problem hiding this comment.
Would converting this to a Vec<(Project, Version) cause issues somewhere?
I thing that changing this before the project grows too big could be beneficial, as making sure that both vecs are in sync when we have to drop values from within (removing duplicates for example) can be difficult
There was a problem hiding this comment.
Yes I did think about doing this too
There was a problem hiding this comment.
Yes because this is not a sorted map. I specifically asked Modrinth on this:
For example, if I have a dependency on any version of Fabric API and a dependency on version 4.0.0 of Mod Menu, it will have both Fabric API and Mod Menu in the projects array and version 4.0.0 of Mod Menu in the versions array
If another version starts depending on version 4.0.1 of Mod Menu then Mod Menu 4.0.1 will appear in the versions array in addition to 4.0.0
That means, the two Vec's can have different lengths. Also they said that I shouldn't rely on their sorting.
|
|
||
| async fn get_versions(&self, ids: Vec<(&ID, &ID)>) -> Result<Vec<Version<ID>>, E> { | ||
| let mut versions = Vec::new(); | ||
| for id in ids { |
| pub size: Option<usize>, | ||
| } | ||
|
|
||
| impl Asset { |
There was a problem hiding this comment.
That sounds like a good place to put a builder in
Either make it manually, or there's a whole lot of procedural macros that can do the job
There was a problem hiding this comment.
I've never heard of that, but yeah a better implementation of the contructors for this struct is sorely needed
| #[derive(Deserialize, Serialize, Debug, Clone)] | ||
| pub struct VersionDependency<ID> { | ||
| /// The ID of the project to depend on | ||
| pub project_id: Option<ID>, |
There was a problem hiding this comment.
What relation is there between the project_id and the version?
If it is exclusive (one OR the other), an enum VersionDescriptor could be interesting to make it explicit, and if both can / must be defined, then this system works finely
There was a problem hiding this comment.
On Modrinth, they're exclusive but on CurseForge they're not. CurseForge does this extremely annoying thing where you cannot use a file id independently, you have to include the project id with it.
This is also why the version calls have rather confusing parameters, because both are required (only for cf)
| use async_trait::async_trait; | ||
| use ferinth::Ferinth; | ||
|
|
||
| pub type ID = String; |
There was a problem hiding this comment.
That took me quite a bit of time to find 🙃 lol
| pub type ID = String; | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { |
There was a problem hiding this comment.
After splitting into crates, it will namespaced as dotium_mr::Error which is the way I always do errors. I find that every other crate always uses Error with no modifications which is why I do that too.
| )) | ||
| } | ||
|
|
||
| async fn get_versions(&self, ids: Vec<(&ID, &ID)>) -> Result<Vec<Version<ID>>, Error> { |
There was a problem hiding this comment.
Maybe explicit the type with something like:
pub struct VersionDescriptor(&ID, &ID)along with a rustdoc to make the definition clearer
There was a problem hiding this comment.
Yup the rustdocs will explain this properly.
|
|
Currently only the modrinth module is implemented