Feature/uri #11

brandonphelps · 2021-01-16T03:22:36Z

Goal here is to provide a center for all end points and a builder for end points.

Not all the end points are flushed out and some of the builder needs the ability to join end points together. however if far enough to be useful for some of the endpoints and I'll be looking to integrate the api key thing.

added generated Cargo.toml and src dir

them based on input arguments.

wishing to make requests from the various endpoints

dailycrafting, account dungeons, account dies, item stats, recipes, item stats all

alexFickle

In general things really need their intent documented.

alexFickle · 2021-01-16T19:48:01Z

src/uri.rs

+// should be usize?
+
+// todo: move this to some types thing?
+pub struct ItemId(pub u128);


u128 is really overkill for this. All of these will fit in a u32.

I'm working on a sqlite3 caching feature and it does not support u128 or (currently) u64 as keys.

yeah i'm up for w/e the size should be, didn't really see anything on the wiki that specified an upper limit.
I doubt we'd need 128 sized items, but would be cool to have the caching support it imo

alexFickle · 2021-01-16T19:49:20Z

src/uri.rs

+pub struct ApiKey(pub String);
+pub struct AchievementId(pub u128);
+
+#[allow(non_camel_case_types)]


We should remove this and fix the name of all enum variants that are triggering this warning. We should be following the default Rust style guides.

Hmm, yeah initially I was hoping for some sort of macro solution that could work on the names of the enumed items. ie. account_materials would resolve to account/materials or something. using snake seemed easier to me to implement such a thing, granted the value in determining such a system might not be worth it.

alexFickle · 2021-01-16T19:57:48Z

src/uri.rs

+}
+
+#[derive(Deserialize, Debug)]
+struct ItemDetails {


This looks like it has the additional item attributes that only armor has. Not all items will have this. ItemDetails should probably be a sum type based on the item type. Something like

enum ItemDetails { Armor(ArmorDetails), Weapons(WeaponDetails), // etc... }

This does not seem needed for this feature anyway, so should probably be deleted. Even if we are going to be deserializing items to test the URI building we do not need to fully test the complicated data model.

I don't think it should be deleted as that sounds like its throwing away work. These are here as a implementation for what the api could be.
I think at some point they'd be moved into a models.rs section that would contain the deserialized types from the api.

I do agree that the format should be better suited like you are indicating.

alexFickle · 2021-01-16T20:00:29Z

src/uri.rs

+    details: ItemDetails,
+}
+
+// should be usize?


Using usize for values coming from a web API is iffy to me. Introduces different platforms treating the same data slightly differently for no real compelling reason.

ah sure, this was here as a question of not really know what the range of values from the api would be returned as.

alexFickle · 2021-01-16T20:10:06Z

src/uri.rs

@@ -0,0 +1,261 @@
+#![allow(dead_code)]
+
+use reqwest;


Per the build output this import is not used. Delete.

Its currently used within the tests, so i'll move it there.

alexFickle · 2021-01-16T20:20:38Z

src/uri.rs

+
+pub struct Requester {
+    version: ApiVersion,
+    api_key: Option<ApiKey>,


This attribute is currently unused.

Also I have some use cases where I will be requesting data from many different users. Making a Requester per api key may be kinda odd. Really depends on the goal of this type.

If a publicly facing (Rust) API requires an API key I'd prefer that it explicitly takes it as a parameter (and also documents the permission requirements). An optional API key in a requester seems like we will be making requesters per request (which eliminates the potential value add of injecting the base_uri) or we will be sneaking a Option<ApiKey> through all of our code, potentially doing the wrong thing.

Could you expand on how have a Reqeuster per api key seems odd? That seem normal to me, as the binding of a Requester to the api key allows for building uris for that specific key.

I picture a sort of API builder that constructs apis based on conveniently provided input from the user, having to have the user specify their api key on each request sounds cumbersome to me, hence the key can be stored within the Requester type.

Its ment as optional due to not all the api end points require authentication and thus this is to allow for a user to request api end points that do not require authentication.

Sounds like you are suggesting to go towards a compile time check of api key and its permissions per end point? That sounds good me. Not certain what you mean by potentially doing the wrong thing though.

alexFickle · 2021-01-16T20:25:49Z

src/uri.rs

+    }
+}
+
+pub struct Requester {


This type is named Requester yet makes no requests. What is the goal of this type? It should be documented so it can be discussed.

Currently it seems that this type may not be needed. The only value of its state is specifying a different base url or API version. The base url may have some value, especially for testing. Although to me it makes more sense that the dependency injection happen at a much higher layer then uri building. The API version will never be changeable without huge changes to the library (assuming that a new version of the API ever happens).

If eventually it will be making requests then indicate so. There is a lot of design work to talk about in this area.

Could you elaborate on the dependency injection statement? What dependency are you talking about the base url? I'd picture that the version if accounted for in the beginning of the libraries design would mean that there would be little to zero library changes.

I do doubt they'll change the versions however.
The documentation is certainly needed along with the intents, no doubt bout that.

alexFickle · 2021-01-16T20:30:32Z

src/uri.rs

+
+// these item structs should be namespaced into the v2 api.
+#[derive(Deserialize, Debug)]
+struct ItemAttribute {


All of these Item types are not publicly accessible or used in non-test code. If these are intended for testing then they should be moved to the testing module. However, testing URI building does not really require fully testing the deserialization of the received data.

Yeah, I can see moving them into the testing section.
And yeah they aren't directly needed for testing uri building, but it certainly cooler, to actually see data be returned and deserialized, also helps with my understanding of how the libraries interact which each other. I picture the final tests for the building of uris will not be making requests from the server or need the server at all.

alexFickle · 2021-01-16T20:51:03Z

src/uri.rs

+pub struct AchievementId(pub u128);
+
+#[allow(non_camel_case_types)]
+pub enum EndPoint {


Documentation? What is the goal of this type. Is it to exactly model the gw2 API URIs? Is it to be the Requestor's request interface? Depending on the answer I'd recommend different design changes.

The basics is that its to capture all of the different end points of the gw2 api as well any helpeful information about each of the end points. note the functions for getting the uri strings as well as information like if authentication is required. With the additions of the various api keys i'd imagen it would also return out the required permissions per api end point or have a compiled time checked api for if a specific key is okay to be used for that end point. I'm also looking to have this provide utility functions for bundling batches of requests into a single request.
Such as
item1 = items/1, item2 = items/2 then if a request wants say [item1, item2] this is distilled into a single request to the server rather than 2, then the items are returned as expected maintain a 1 to 1 return.

src/uri.rs

switched the enum types to rust type format standards

…point

Brandon Phelps added 14 commits January 5, 2021 19:54

Merge pull request #1 from brandonphelps/develop

1889e01

added generated Cargo.toml and src dir

added some stuff for listing out the uri end points and constructing

5f51f8f

them based on input arguments.

added in more uri end poitns, recipes and item stats

0790ec9

Merge remote-tracking branch 'origin/develop' into feature/uri

0b37b20

Updated the requires auth with more of the end points

ea19d68

fixed move p

188a738

added in a requester that will conslidate the parameters of a user

bc8fe2a

wishing to make requests from the various endpoints

removed testing function and added todos

c5cad88

added in reqwest and serialization for item

4a030b2

added in a few more structs for deserialization

15587e8

added in achivements and achivements ids end point

cb4aaea

Added in account, account achivements, account mats, account

276f085

dailycrafting, account dungeons, account dies, item stats, recipes, item stats all

Merge remote-tracking branch 'origin/develop' into feature/uri

ca57949

ran fmt

25d1344

alexFickle requested changes Jan 16, 2021

View reviewed changes

brandon phelps added 2 commits January 17, 2021 10:24

added a todo / comment

491882c

moved some use statements,

aa375d9

switched the enum types to rust type format standards

brandonphelps force-pushed the feature/uri branch from a3b3353 to aa375d9 Compare January 17, 2021 18:30

changed the ItemsIds and such type to u32, added Option for Items end…

97085d7

…point

brandonphelps requested a review from alexFickle January 17, 2021 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/uri #11

Feature/uri #11

brandonphelps commented Jan 16, 2021 •

edited

Loading

alexFickle left a comment

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021 •

edited

Loading

alexFickle Jan 16, 2021

brandonphelps Jan 17, 2021 •

edited

Loading

Feature/uri #11

Are you sure you want to change the base?

Feature/uri #11

Conversation

brandonphelps commented Jan 16, 2021 • edited Loading

alexFickle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonphelps Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonphelps Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

brandonphelps commented Jan 16, 2021 •

edited

Loading

brandonphelps Jan 17, 2021 •

edited

Loading

brandonphelps Jan 17, 2021 •

edited

Loading