Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V2 bringup #4

Merged
merged 8 commits into from
Feb 3, 2022
Merged

V2 bringup #4

merged 8 commits into from
Feb 3, 2022

Conversation

theRookieCoder
Copy link
Collaborator

A PR extension (?) of #3

@theRookieCoder theRookieCoder mentioned this pull request Feb 3, 2022
@theRookieCoder
Copy link
Collaborator Author

theRookieCoder commented Feb 3, 2022

I think we can rework this commit after transitioning to url (I'm doing that right now)

@theRookieCoder
Copy link
Collaborator Author

theRookieCoder commented Feb 3, 2022

I just realised the doctests are completely broken they just pass all the time

Edit: It seems that errors that occur in tokio_test::block_on() aren't caught at all they're just ignored! This is because the result of the async block is ignored 🤦🏻‍♂️. But it seems panics in the async block do cause tokio-test to panic

Edit: I'm just doing this now

/// # #[tokio::main]
/// # async fn main() -> Result<(), ferinth::Error> {
/// ...actual test...
/// # Ok(()) }

Not checking Results is quite an oversight for a testing library imo

@4JX
Copy link
Contributor

4JX commented Feb 3, 2022

I think we can rework this commit after transitioning to url (I'm doing that right now)

Good idea. On that note I'd prefer for the "modloader" enum be placed somewhere more global. I did consider also using it for the loaders section of the Version struct, but since no guarantees seem to be given for what might appear there I decided against it.

@theRookieCoder
Copy link
Collaborator Author

I think the intended way to specify/read mod loaders is to get them from the mod loaders tag

- Remove `tokio-test` dependency
- All doctests now use a `#[tokio::main]` runtime instead of `tokio_test::block_on()`
- Add a `URLParseError` which is wrapper of `url::ParseError`
- Removed `request_rel`. You're expected to use `join()` on the `API_URL_BASE`
- Created a lazy static `url::Url` `API_URL_BASE`
- All api call functions have transitioned from `format!(...)` to `API_URL_BASE.join(...)`
- Updated tag api calls to the v2 API
- Tags now have their own structs
- All enums now have `#[serde(rename_all = "lowercase")]`
- All structs now have  `#[serde(deny_unknown_fields)]`
- Add the example project to the workspace
- Improved documentation
@theRookieCoder
Copy link
Collaborator Author

I've done pretty much everything needed to release v2. Any suggestions @4JX?

@theRookieCoder theRookieCoder marked this pull request as ready for review February 3, 2022 12:28
@4JX
Copy link
Contributor

4JX commented Feb 3, 2022

I've done pretty much everything needed to release v2. Any suggestions @4JX?

Would still fancy filtering for list_versions to be a part of the release, maybe with just a Vec<String> for the loaders part rather than an enum?

It seems more convenient than doing the same thing with code on the program later on given the API provides support for it.

@theRookieCoder
Copy link
Collaborator Author

theRookieCoder commented Feb 3, 2022

Ah I see I thought we'd make a seperate pr for that and mod searching. Would releasing that in v2.1 be okay, or you seriously need that feature now?

@4JX
Copy link
Contributor

4JX commented Feb 3, 2022

Ah I see I thought we'd make a seperate pr for that and mod searching. Would releasing that in v2.1 be okay, or you seriously need that feature now?

Actually yes sounds good 👍. Go for it.

@theRookieCoder
Copy link
Collaborator Author

I'll migrate ferium to this to test for any edge cases first. Is a changelog necessary? I mean it's a major release so obviously everything has changed

@4JX
Copy link
Contributor

4JX commented Feb 3, 2022

I'll migrate ferium to this to test for any edge cases first. Is a changelog necessary? I mean it's a major release so obviously everything has changed

Other than "you should probably substitute mod with project in your function calls" you can probably do a mobile dev changelog and throw everything else under "various improvements"

@theRookieCoder
Copy link
Collaborator Author

you can probably do a mobile dev changelog and throw everything else under "various improvements"

Haha I was thinking of just doing "Initial release" and maybe linking this PR as a 'changelog'

@theRookieCoder
Copy link
Collaborator Author

Ferium works fine! Just had to change get_mod to get_project and edit a few lines to adapt to the new struct defs

@theRookieCoder theRookieCoder merged commit f190626 into master Feb 3, 2022
@theRookieCoder theRookieCoder deleted the v2-bringup branch February 3, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants