Skip to content
This repository was archived by the owner on Dec 26, 2022. It is now read-only.

Conversation

@beastmatser
Copy link
Contributor

Changes

This adds a permission handler which can convert integers in the correct permission and vice versa.

Check off the following

  • I have tested my changes with the current requirements
  • My Code follows the pep8 code style.

@beastmatser
Copy link
Contributor Author

beastmatser commented Jan 10, 2022

#197
This pull request is accidentally continued from #377 (changes undone)

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #378 (b9688c6) into main (d2b1e22) will increase coverage by 0.63%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   90.54%   91.17%   +0.63%     
==========================================
  Files           8        9       +1     
  Lines          74      102      +28     
==========================================
+ Hits           67       93      +26     
- Misses          7        9       +2     
Impacted Files Coverage Δ
tests/objects/guild/test_permission.py 92.85% <92.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2b1e22...b9688c6. Read the comment docs.

@zunda-arrow
Copy link
Member

zunda-arrow commented Jan 11, 2022

Looking over this, I think an implementation similar to this would be better https://github.com/Pincer-org/Pincer/blob/main/pincer/objects/events/presence.py#L134.
edit: Actually that doesn't work with None so that has to be ruled out

@beastmatser
Copy link
Contributor Author

Looking over this, I think an implementation similar to this would be better https://github.com/Pincer-org/Pincer/blob/main/pincer/objects/events/presence.py#L134. edit: Actually that doesn't work with None so that has to be ruled out

I feel like the whole purpose of this class is to ease the use of permissions, wouldn't a similar implementation to this kind of defeat the purpose of this class

from typing import Tuple, Optional


class PermissionEnums(Enum):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class PermissionEnums(Enum):
class Permissions(Enum):

@zunda-arrow
Copy link
Member

Ignore what I said about |=

Co-authored-by: Lunarmagpie <[email protected]>
Co-authored-by: Lunarmagpie <[email protected]>
@Sigmanificient Sigmanificient requested a review from trag1c January 11, 2022 23:47
Copy link
Member

@Enderchief Enderchief left a comment

Choose a reason for hiding this comment

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

I agree with trag1c

Comment on lines 202 to 203
elif isinstance(object, tuple):
return self.to_int() == object
Copy link
Member

Choose a reason for hiding this comment

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

I not sure there is any scenario where int can be equal to tuple.

Suggested change
elif isinstance(object, tuple):
return self.to_int() == object

Copy link
Member

Choose a reason for hiding this comment

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

The function returns a tuple of ints. Maybe the function name should be changed because I was confused by this too.

Copy link
Contributor Author

@beastmatser beastmatser Jan 13, 2022

Choose a reason for hiding this comment

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

A few ideas:

  • to_ints
  • to_tuple
  • convert

I changed it to_tuple for now

Copy link
Member

Choose a reason for hiding this comment

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

I think you should do to_ints since it matches with from_ints

My other idea is to_permission_tuple but thats kinda long.

Copy link
Member

@zunda-arrow zunda-arrow left a comment

Choose a reason for hiding this comment

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

Actually do what tragic says then I will approve

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 14, 2022

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 2.02%.

Quality metrics Before After Change
Complexity 0.75 ⭐ 0.20 ⭐ -0.55 👍
Method Length 166.67 😞 127.75 😞 -38.92 👍
Working memory 5.50 ⭐ 5.60 ⭐ 0.10 👎
Quality 84.75% 82.73% -2.02% 👎
Other metrics Before After Change
Lines 220 206 -14
Changed files Quality Before Quality After Quality Change
pincer/objects/init.py 49.87% 😞 49.81% 😞 -0.06% 👎
pincer/objects/guild/init.py 69.07% 🙂 68.69% 🙂 -0.38% 👎
pincer/objects/guild/overwrite.py 93.18% ⭐ 98.39% ⭐ 5.21% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@Sigmanificient Sigmanificient merged commit 1958975 into Pincer-org:main Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants