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

@jack1142
Copy link
Contributor

commented Feb 27, 2019

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.

@mikeshardmind
Copy link
Member

left a comment

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.

@jack1142

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@jack1142

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I added it then.

@jack1142

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@jack1142

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

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

@Tobotimus Tobotimus added the Type: Fix label Mar 4, 2019

@Tobotimus

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@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)
@jack1142

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

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

@jack1142

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

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

@Tobotimus
Copy link
Member

left a comment

Thanks @jack1142!

@Tobotimus Tobotimus added QA: Passed and removed QA: Unassigned labels Mar 4, 2019

@Tobotimus Tobotimus merged commit 30fa930 into Cog-Creators:V3/develop Mar 4, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jack1142 jack1142 deleted the jack1142:V3/spelling_correction branch Mar 4, 2019

jack1142 added a commit to jack1142/Red-DiscordBot that referenced this pull request Apr 24, 2019

tekulvw added a commit that referenced this pull request Apr 25, 2019

[Docs] Changelog entries for contributions by jack1142 (#2612)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.