If the user is using a type checker, they would already see it only accepts str.

For attributes yes, but the methods do not strictly only take strings as they are typed with Any unlike what the docs say, so this change is aimed at making the typing more concise.

If they are not using a type checker, not noticing it is str would also mean they do not notice SupportsStr.

This change is aimed towards people who use type checkers, as it is not enforced at runtime.

Having to inherit an ABC removes almost all uses of a non-str argument that should already be str anyway.

While I don't think of this as that big of an issue as the user can just convert any non-str arguments into strings using str(), unfortunately this is just how it is, as I'm not aware of any way to fix this flaw.

Because object implements __str__, all classes also implement __str__. SupportsStr is effectively the same as Any (actually object, but that doesn't matter here). I don't think there is anything to be gained from this change.

Because object implements __str__, all classes also implement __str__

But what about any objects the user creates? SupportsStr is meant to allow custom objects to be passed as args.

Though looking back on it nextcord classes wont be accepted by the type checker as they don't inherit from SupportsStr, so I would need to go through every class that does implement __str__ and make them inherit from SupportsStr, but I don't think that solution would be accepted. Unfortunately this is a flaw of this solution as SupportsStr needs to be inherited for the type checker to know.

SupportsStr is effectively the same as Any (actually object, but that doesn't matter here).

I have to disagree with this, SupportsStr allows the type checker to see if a class has manually implemented __str__, while Any allows any value to be passed, which is the problem this pr aims to solve.

I get this change is odd, I myself do not like it as it isn't a very clean solution, but this is the only solution I could come up with to close this issue. If you or anyone else have any ideas on how I can improve this pr I'm all ears, if not then this pr can be closed.

For attributes yes, but the methods do not strictly only take strings as they are typed with Any unlike what the docs say, so this change is aimed at making the typing more concise.

Then I think they should be typed as str instead.,

While I don't think of this as that big of an issue as the user can just convert any non-str arguments into strings using str(), unfortunately this is just how it is, as I'm not aware of any way to fix this flaw.

What I meant was that I could not think of any form of class which would satisfy SupportsStr or would be worth inheriting it for, instead of just using str() already.

Then I think they should be typed as str instead.

What I meant was that I could not think of any form of class which would satisfy SupportsStr or would be worth inheriting it for, instead of just using str() already.

I agree, this is probably the best solution, but would doing this still satisfy the issue? If so I'll push those changes.

I mistook the SupportsStr class for a typing.Protocol (where my criticisms apply), my bad.

But then I don't like this change because of programming ergonomics, à la "... if it quacks like a duck, then it probably is a duck":

Overriding __str__ is not the only way to influence the output of __str__, changing the __repr__ also changes __str__. So every class that only implements __repr__ has to implement a dummy method something like this:

def __str__(self) -> str:
    return repr(self)

I think we have 2-3 possible options instead:

1. accept all objects and just call str() on them (no changes)
2. to avoid breaking changes, change the typehint to str but keep the same logic
3. change the type to str and remove the call to str()

I think 2 or 3 is the right course of action here, while I'm personally leaning towards 2.

I think we have 2-3 possible options instead:

1. accept all objects and just call str() on them (no changes)
2. to avoid breaking changes, change the typehint to str but keep the same logic
3. change the type to str and remove the call to str()

I think 2 or 3 is the right course of action here, while I'm personally leaning towards 2.

Same here, 2 is probably the way to go.

@EmreTech can I get your thoughts on which option would be best? (Apologies for the mention)

I think we have 2-3 possible options instead:

1. accept all objects and just call str() on them (no changes)
2. to avoid breaking changes, change the typehint to str but keep the same logic
3. change the type to str and remove the call to str()

I think 2 or 3 is the right course of action here, while I'm personally leaning towards 2.

Same here, 2 is probably the way to go.

@EmreTech can I get your thoughts on which option would be best? (Apologies for the mention)

I would go with option 3 since we're working on 3.0. Breaking changes don't matter all that much.

I would go with option 3 since we're working on 3.0. Breaking changes don't matter all that much.

Alright, I'll begin work on that then.

Forgot to mention but option 3 was implemented in the above commits, feel free to review.

Has decent testing been done on all modified functions and things that call them? I wouldn't be surprised if NC internally relies on str() being used.

Has decent testing been done on all modified functions and things that call them? I wouldn't be surprised if NC internally relies on str() being used.

I wasn't aware of any internals calling these functions, I'll look into it though, thanks.

I'm unsure if anything calls them internally, just wanted to check if you looked into that already or not.

Also, the PR summary probably should get updated? I'm not seeing a SupportsStr anywhere.

Also, the PR summary probably should get updated? I'm not seeing a SupportsStr anywhere.

Yeah, I'll get on that soon.

Has decent testing been done on all modified functions and things that call them? I wouldn't be surprised if NC internally relies on str() being used.

Looked into the internals as you requested, I didn't find any internal code calling these functions so this pr should be fine to merge. If i'm wrong and there are internals calling these functions, please do let me know.

Alright, so I did find one slight issue. /examples/custom_interaction.py calls embed.set_author(), but gives the icon_url kwarg an Asset object, rather than a string. Asset just returns self._url when str()'d, but with the str() removed I don't think that would work anymore.

Super easy fix for people, just add .url to the asset being passed in, but that does make this a breaking change. I would like to see someone knowing that this is a breaking change approve this (@EmmmaTech) and the example file I mentioned above be fixed before merging this in.

Is passing an Asset object into icon_url common? I know I don't do it, but I also don't use Asset objects very often. Are there other parts of Nextcord with "image url" kwargs that accept Asset objects, and thus this PR would make Embeds inconsistent with the rest of the lib?

I'm aware such a typing change would be breaking

Alright, so I did find one slight issue. /examples/custom_interaction.py calls embed.set_author(), but gives the icon_url kwarg an Asset object, rather than a string. Asset just returns self._url when str()'d, but with the str() removed I don't think that would work anymore.

Thanks for informing me, I'll commit the fix soon.

Is passing an Asset object into icon_url common? I know I don't do it, but I also don't use Asset objects very often. Are there other parts of Nextcord with "image url" kwargs that accept Asset objects, and thus this PR would make Embeds inconsistent with the rest of the lib?

I don't think there are any methods that strictly accept an Asset object for a: "image_url", "url" parameter. From what I know they accept str, so this pr shouldn't make embeds inconsistent.