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

fix: implement valueOf on pseudo managers #4595

Merged
merged 2 commits into from Dec 14, 2020

Conversation

almostSouji
Copy link
Member

Please describe the changes this PR makes and why it should be merged:

Currently BaseManager#valueOf returns the cache collection, while this is not the case for the pseudo managers GuildEmojiRoleManager and GuildMemberRolemanager.

This PR brings the needed consistency and implements the pseudo managers #valueOf method akin to the BaseManager.

Versioning considerations:

  • This could be seen as semver major as it changes the behavior of the public facing API #valueOf to be consistent with BaseManager
  • This could be seen as patch since it fixes inconsistent behavior, which appeared due to an oversight (as already briefly mentioned in fix(BaseManager): properly type valueOf #4594 the return value for BaseManager was picked to properly allow passing the structure through broadcast evaluation without loosing all its data)
  • "This PR changes the library's interface" is not ticked due to #valueOf already being inherited from the Object base
  • "This PR includes breaking changes" is not ticked due to above unclear considerations

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@almostSouji
Copy link
Member Author

Another (probably preferable) solution @1Computer1 has brought up would be to remove pseudo managers and instead make cache a getter so the manager does not necessarily need to carry its own cache (which is the current reason for pseudo managers to exist).

While technically that could be framed as semver major the argument could be made that it's really a fix for an oversight and that .cache (which is currently mutable) should not be mutated to begin with (to guarantee correct behavior).

@iCrawl iCrawl requested a review from SpaceEEC December 12, 2020 14:01
@iCrawl iCrawl merged commit 8883a01 into discordjs:master Dec 14, 2020
@almostSouji almostSouji deleted the pseudomanager-valueof branch December 14, 2020 16:52
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.

None yet

5 participants