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

Spelling correction of method name in Tunnel #2496

Merged
merged 3 commits into from Mar 4, 2019

Conversation

Jackenmen
Copy link
Member

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Fix for wrong spelling in method name files_from_attatch, I changed it to files_from_attach in all its appearances.

This is actually breaking change, because of change of method's name, lol.

Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Choose a reason for hiding this comment

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

I'd prefer if this was done in a non-breaking fashion for now given how much QA has recommended people use this method when they need files in a quick and easy way.

    @property
    def files_from_attatch(self):
        return self.files_from_attach

Adding this to the Tunnel class with your changes would be sufficient for that, the property could then be removed later on.

@Jackenmen
Copy link
Member Author

Jackenmen commented Mar 1, 2019

Because it is static method this would still be breaking change, because now you would have to create instance before using the method. I think it should be enough to just do a simple assignment to old name after defining the method.

files_from_attatch = files_from_attach

@mikeshardmind
Copy link
Contributor

Right, forgot that was made a static method for a moment.

@Jackenmen
Copy link
Member Author

I added it then.

@Jackenmen
Copy link
Member Author

Jackenmen commented Mar 1, 2019

async def react_close(self, *, uid: int, message: str = ""):
send_to = self.origin if uid == self.sender.id else self.sender
closer = next(filter(lambda x: x.id == uid, (self.sender, self.recipient)), None)
await send_to.send(filter_mass_mentions(message.format(closer=closer)))

also, I wanted to add PR for this, because it looks like bad logic to me, this will always send the message to the side that started the tunnel, either original channel, or the sender that the bot reacts to on this channel - I think it should be:

send_to = self.recipient if uid == self.sender.id else self.origin

I just thought that maybe I could do it in this PR (assuming this isn't some strange logic that I just don't understand)

@mikeshardmind
Copy link
Contributor

your logic is solid there and properly fixes the intent (of sending a message of the tunnel being closed to the other end)

@Jackenmen
Copy link
Member Author

Can I do it here or should I just do another PR?

@Tobotimus
Copy link
Member

@jack1142 Another PR would be great for that change.

As for this PR, could you just add this comment above line 161?

# Backwards-compatible typo fix (GH-2496)

@Jackenmen
Copy link
Member Author

Ayy, I wanted to do that from the beginning (I wouldn't have thought of adding PR number though), I'll commit it in a second

@Jackenmen
Copy link
Member Author

@Tobotimus updated, I'll add the other PR later today.

Copy link
Member

@Tobotimus Tobotimus left a comment

Choose a reason for hiding this comment

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

Thanks @jack1142!

@Tobotimus Tobotimus added QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). and removed QA: Unassigned labels Mar 4, 2019
@Tobotimus Tobotimus merged commit 30fa930 into Cog-Creators:V3/develop Mar 4, 2019
@Jackenmen Jackenmen deleted the V3/spelling_correction branch March 4, 2019 19:53
Jackenmen added a commit to Jackenmen/Red-DiscordBot that referenced this pull request Apr 24, 2019
tekulvw pushed a commit that referenced this pull request Apr 25, 2019
* docs(changelog): [Mod] Allow admin to choose amount of repeats for "deleterepeats" #2437

* docs(changelog): Spelling correction of method name in Tunnel #2496

* docs(changelog): Tunnel fix - When tunnel closes, message should be sent to other end #2507

* docs(changelog): [V3 Downloader] Tell user how to load the cog after [p]cog install #2523

* docs(changelog): [V3 Audio] If bot has move members perm, it can join to user-limited channels #2525

* docs(changelog): [Trivia] Fix of dead image link (world flags) #2540

* docs(changelog): [V3 Test] Make sure that trivia test will use utf-8 encoding #2565

* docs(changelog): [V3 Core] Print actual version, when `--version` flag is used #2567

* docs(changelog): [V3 Downloader] Stop including subpackages in cog list #2590

* docs(changelog): [V3 Downloader] Uninstall multiple cogs #2592

* docs(changelog): [V3 Downloader] Always remove cog from installed in `[p]cog uninstall` #2595
@Jackenmen Jackenmen added this to the 3.1.0 milestone Nov 17, 2020
@Jackenmen Jackenmen added the Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing. label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants