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

Allow specifying URL for GDPS #49

Closed
wants to merge 8 commits into from
Closed

Conversation

yuptune
Copy link

@yuptune yuptune commented Jan 11, 2025

_This is exactly like the one before, but with needed changes that were put in here.

Please note that I don't even know what I'm doing. So if this is wrong, PLEASE correct me. It would be highly appreciated._

Thanks,
yuptune 👍

LG125YT and others added 3 commits January 10, 2025 14:48
…o edit URLs for GDPSes

The const was changed to a static of type ``OnceCell<&'static str>``.
``get_gd_url`` is now a call to ``OnceCell::get_or_init``, and the "or init" part is also set to the boomlings servers. (This is changeable, by the way. ;)). Also, i don't even know what i'm doing. So if i'm wrong please correct the mistakes i made in this commit.

Hope this helps,
yuptune
@yuptune
Copy link
Author

yuptune commented Jan 11, 2025

I just realized I put the note for the PR inside of the commit. Silly me

Copy link
Owner

@stadust stadust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please drop all the unrelated deletion of comments/module declarations (I guess those are from testing?).

Also, now that dash-rs is gaining this feature, maybe we should add a quick note in the readme telling people how to use this? (explicitly setting the OnceLock from fn main)

Comment on lines 11 to 40
macro_rules! const_setter {
($name: ident, $field: ident, $t: ty) => {
pub const fn $name(mut self, $field: $t) -> Self {
self.$field = $field;
self
}
};

($name: ident, $t: ty) => {
pub const fn $name(mut self, arg0: $t) -> Self {
self.$name = arg0;
self
}
};
static GDPS_URL: OnceCell<String> = OnceCell::new();

($(#[$attr:meta])* $name: ident: $t: ty) => {
$(#[$attr])*
pub const fn $name(mut self, $name: $t) -> Self {
self.$name = $name;
self
}
};

($(#[$attr:meta])* $field:ident[$name: ident]: $t: ty) => {
$(#[$attr])*
pub const fn $name(mut self, $field: $t) -> Self {
self.$field = $field;
self
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you accidentally deleted a bit more than intended :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the commit says its outdated for some reason. i don't know why.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, seems like now these are moved to the end of the file. Can you move them back?

pub mod level;
pub mod user;

pub const REQUEST_BASE_URL: &str = "https://www.boomlings.com/database/";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, maybe let's keep this constant, but rename it BOOMLINGS_REQUEST_BASE

self
}
};
static GDPS_URL: OnceCell<String> = OnceCell::new();
Copy link
Owner

@stadust stadust Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be a pub static, so that code using dash-rs can set the OnceCell in their initialization code.

Also, let's name it REQUEST_BASE, because it doesn't have to be a GDPS URL (and in the most common usecase of interacting with the boomlings servers, it actually won't be a GDPS URL).

I also realized that this needs to be a OnceLock and not a OnceCell for use in statics. Sorry! So altogether, this should be

Suggested change
static GDPS_URL: OnceCell<String> = OnceCell::new();
pub static REQUEST_BASE: OnceLock<&'static str> = OnceLock::new();

@stadust
Copy link
Owner

stadust commented Jan 11, 2025

Btw, you can reuse PRs by just pushing to the branch from which you originally opened the PR. GitHub will automatically update the PR, so you don't have to open a new one :)

@yuptune
Copy link
Author

yuptune commented Jan 11, 2025

Alright, thanks :P

@yuptune
Copy link
Author

yuptune commented Jan 11, 2025

Alright, i think i fixed all of the issues you listed.

@yuptune
Copy link
Author

yuptune commented Jan 11, 2025

20250110_112340

@stadust stadust changed the title New PR. :) Allow specifying URL for GDPS Jan 11, 2025
Copy link
Owner

@stadust stadust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing all those typos!

I unresolved some of the older reviews that don't seem fixed. Maybe you forgot to push something? :o

README.md Outdated
Comment on lines 12 to 13
`rust
GDPS_URL.get_or_init(|| "https://your-custom-gdps-url.com".to_string());`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`rust
GDPS_URL.get_or_init(|| "https://your-custom-gdps-url.com".to_string());`
```rust
REQUEST_BASE.get_or_init(|| "https://your-custom-gdps-url.com");`
\```
in your `fn main` (or anywhere before the first call to any dash-rs functions)

(without the backslack at the end, turns out putting backticks into codeblocks is quite hard haha)

Comment on lines 11 to 40
macro_rules! const_setter {
($name: ident, $field: ident, $t: ty) => {
pub const fn $name(mut self, $field: $t) -> Self {
self.$field = $field;
self
}
};

($name: ident, $t: ty) => {
pub const fn $name(mut self, arg0: $t) -> Self {
self.$name = arg0;
self
}
};
static GDPS_URL: OnceCell<String> = OnceCell::new();

($(#[$attr:meta])* $name: ident: $t: ty) => {
$(#[$attr])*
pub const fn $name(mut self, $name: $t) -> Self {
self.$name = $name;
self
}
};

($(#[$attr:meta])* $field:ident[$name: ident]: $t: ty) => {
$(#[$attr])*
pub const fn $name(mut self, $field: $t) -> Self {
self.$field = $field;
self
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, seems like now these are moved to the end of the file. Can you move them back?

Comment on lines -43 to -45
pub mod comment;
pub mod level;
pub mod user;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These module declarations still seem to be deleted :(

self
}
}
pub fn get_gdps_url() -> String {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also rename this function to something like fn base_request_url()

README.md Outdated
@@ -7,6 +7,11 @@

The project is a collaboration with [mgostIH](https://github.com/mgostIH), on whose idea the initial library design is based on and who continues to provide incredibly helpful insights into optimization, Geometry Dash and Rust.

## Guide
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Guide
## Using dash-rs with a GDPS

README.md Outdated
@@ -7,6 +7,11 @@

The project is a collaboration with [mgostIH](https://github.com/mgostIH), on whose idea the initial library design is based on and who continues to provide incredibly helpful insights into optimization, Geometry Dash and Rust.

## Guide
If you are planning to use this for your own demonlist that isn't related to the main Geometry Dash servers, you can use:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you are planning to use this for your own demonlist that isn't related to the main Geometry Dash servers, you can use:
If you are planning to use this library for interacting with a GDPS, you can use:

@stadust
Copy link
Owner

stadust commented Jan 13, 2025

Heya, I've done the final fixups locally and squashed everything into ee5919e, which I pushed to master. GitHub won't notice that it's a manual merge, so won't mark this as "merged", but everything is definitely in the master branch now!

Thanks for your contribution!

@stadust stadust closed this Jan 13, 2025
@LG125YT
Copy link
Contributor

LG125YT commented Jan 13, 2025

Ah, it seems i was too late! (I had just finished doing my own fixes and i just committed to the fork repo)

Well, thanks for the review and work anyways!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants