cyperdark / osu-api-extended

Package for advanced work with "osu" api
MIT License
49 stars 15 forks source link

fix mod types and settings #19

Closed yorunoken closed 6 months ago

GabuTheDev commented 9 months ago

Hey there! I suggest making a base interface and extend that for each mod.

GabuTheDev commented 9 months ago

I could do that for you, if you want ofc. (It's your PR after all)

yorunoken commented 9 months ago

yeah that's what I thought of doing but I'm kinda busy rn @GabuTheDev, I'll do it in a bit

yorunoken commented 9 months ago

did you mean something like this?

interface BaseMods {
  acronym: string;
}

interface EZ extends BaseMods {
  acronym: "EZ";
  settings?: {
    retries: number;
  };
}

interface NF {
  acronym: "NF";
}
GabuTheDev commented 9 months ago

did you mean something like this?

Exactly. Should be called BaseMod, and the interface should already have an empty settings property.

yorunoken commented 9 months ago

@GabuTheDev the settings property is only included if there's a non-default setting being applied if the mod doesn't support settings, it simply won't exist, so it's not a good idea to have that as its property

yorunoken commented 9 months ago

you can read all the mods' settings here

GabuTheDev commented 9 months ago

@GabuTheDev the settings property is only included if there's a non-default setting being applied if the mod doesn't support settings, it simply won't exist, so it's not a good idea to have that as its property

My bad. Not so familiar with the osu API. Good to know, thank you!

yorunoken commented 9 months ago

@GabuTheDev no problem!

yorunoken commented 9 months ago

actually, you can just pass in a ? after settings and it'll be fine, thanks for the recommendation @GabuTheDev

yorunoken commented 9 months ago

should be ready to merge now

GabuTheDev commented 9 months ago

@YoruNoKen Your PR will be reviewed by @cyperdark. This might not be merged directly as ck is working on a new version of this package. It also requires a lot of changes to the files that are using the Mod interface currently present in main branch so it might take a while until this change will go live (or maybe I'm wrong, final verdict comes from ck).

yorunoken commented 9 months ago

@GabuTheDev yea ik I talked with ck prior to making this pr, thanks for the attention :D

yorunoken commented 9 months ago

also, from what I can see, Mod is only used to infer a type, so it requires no changes to current files image

cyperdark commented 6 months ago

will be done here https://github.com/cyperdark/osu-api-extended/pull/24

yorunoken commented 6 months ago

sure :)