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

Rename edit_permission_overwrites to edit_permission_overwrite #1195

Merged
merged 13 commits into from
Jul 14, 2022
Merged

Rename edit_permission_overwrites to edit_permission_overwrite #1195

merged 13 commits into from
Jul 14, 2022

Conversation

GoogolGenius
Copy link
Contributor

@GoogolGenius GoogolGenius commented Jun 26, 2022

Summary

Deprecation. Renames edit_permission_overwrites to edit_permission_overwrite.

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

https://discord.com/channels/574921006817476608/700378161526997003/979493994092449802

@GoogolGenius GoogolGenius changed the title Remove plurality of edit_permission_overwrites Rename edit_permission_overwrites to fix incorrect plurality Jun 26, 2022
@parafoxia
Copy link
Contributor

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

@zunda-arrow
Copy link
Contributor

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

The discord API docs also refers to it as "permission overwrites"

@GoogolGenius
Copy link
Contributor Author

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

Hmm I thought and agreed with davfsa that "overwrite" made more sense because it edits a specific entry if I understand the method correctly.

@GoogolGenius
Copy link
Contributor Author

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

The discord API docs also refers to it as "permission overwrites"

This form is used throughout hikari as well but it's this specific method that edits only a single entry I believe for an overwrite?

@GoogolGenius
Copy link
Contributor Author

I'll add the fragment later by the way.

Copy link
Member

@davfsa davfsa left a comment

Choose a reason for hiding this comment

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

This would need deprecation of the old method instead of removal to not turn it into a breaking change. Fragment file should update to reflect that too

@davfsa
Copy link
Member

davfsa commented Jun 26, 2022

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

There is currently an inconsistency in the lib with this name, with one method calling it overwrites and another one overwrite. Since you are editing a specific permission overwrite, I think the singular is more fitting.

This was fact a todo of mine that seems to have been snipped, lol

@GoogolGenius
Copy link
Contributor Author

This was fact a todo of mine that seems to have been snipped, lol

Heh it was, I just thought I could do it anyway haha.

@GoogolGenius
Copy link
Contributor Author

@davfsa Should the tests should use the old name or new name?

@GoogolGenius GoogolGenius requested a review from davfsa June 26, 2022 07:07
@GoogolGenius
Copy link
Contributor Author

GoogolGenius commented Jun 26, 2022

Nvm about review, this is failing CI, let me fix.

Made some silly mistakes lol

@Jonxslays
Copy link
Contributor

Jonxslays commented Jun 26, 2022

@davfsa Should the tests should use the old name or new name?

The old deprecated method and the new method should both be tested up until the deprecated method is removed.

@GoogolGenius
Copy link
Contributor Author

@davfsa Should the tests should use the old name or new name?

The old deprecated method and the new method should both be tested up until the deprecated method is removed.

What about methods that use that method? They can just be updated with the new name and the test changed right?

@parafoxia
Copy link
Contributor

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

There is currently an inconsistency in the lib with this name, with one method calling it overwrites and another one overwrite. Since you are editing a specific permission overwrite, I think the singular is more fitting.

This was fact a todo of mine that seems to have been snipped, lol

Don't you have the ability to edit multiple overwrites with the same method though?

@GoogolGenius
Copy link
Contributor Author

Pretty sure "overwrites" is correct here. The phrase "edit permission overwrite" makes no sense.

There is currently an inconsistency in the lib with this name, with one method calling it overwrites and another one overwrite. Since you are editing a specific permission overwrite, I think the singular is more fitting.
This was fact a todo of mine that seems to have been snipped, lol

Don't you have the ability to edit multiple overwrites with the same method though?

Hmm, now I'm a bit conflicted here. It makes sense as singular when you look at it from an "overwrite entry" but if you're editing multiple permissions, you can use the bitwise | operator on the hikari.Permissions enum. Then it would be edit_overwrite_permissions rather.

@GoogolGenius
Copy link
Contributor Author

I'm also looking at how discord.js and discord.py named this and utterly confused 😕

@davfsa
Copy link
Member

davfsa commented Jun 27, 2022

A permission overwrite is the singular object that consists of a target, what permissions are allowed and which ones are denied.

Since this endpoint only edits a specific one, then edit_permission_overwrite makes more sense than edit_permission_overwrites, as you are only editing one.

I currently can't check and I don't know it of the top of my head, but the method to bulk ediy them is a different one @parafoxia

hikari/api/rest.py Outdated Show resolved Hide resolved
@parafoxia
Copy link
Contributor

A permission overwrite is the singular object that consists of a target, what permissions are allowed and which ones are denied.

Since this endpoint only edits a specific one, then edit_permission_overwrite makes more sense than edit_permission_overwrites, as you are only editing one.

I currently can't check and I don't know it of the top of my head, but the method to bulk ediy them is a different one @parafoxia

Right yeah, I'm seeing what you mean. I'd always taken a "permission overwrite" to be an overwrite of a specific permission, rather than all permissions for a given entity.

@parafoxia
Copy link
Contributor

@GoogleGenius I've spotted a typo which might as well be fixed in this PR. The docs (existing and updated) say "vale" instead of "value" in the allowed and denied fields.

@GoogolGenius
Copy link
Contributor Author

@GoogleGenius I've spotted a typo which might as well be fixed in this PR. The docs (existing and updated) say "vale" instead of "value" in the allowed and denied fields.

Thanks, will fix.

@GoogolGenius GoogolGenius requested a review from davfsa June 27, 2022 17:41
davfsa
davfsa previously approved these changes Jul 5, 2022
@GoogolGenius GoogolGenius changed the title Rename edit_permission_overwrites to fix incorrect plurality Rename edit_permission_overwrites to edit_permission_overwrite Jul 6, 2022
FasterSpeeding
FasterSpeeding previously approved these changes Jul 14, 2022
@GoogolGenius GoogolGenius dismissed stale reviews from FasterSpeeding and davfsa via b6751cb July 14, 2022 16:44
@davfsa davfsa enabled auto-merge (squash) July 14, 2022 16:48
@davfsa davfsa merged commit be5a55f into hikari-py:master Jul 14, 2022
@GoogolGenius GoogolGenius deleted the task/rename-function branch July 14, 2022 22:00
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.

6 participants