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 the URL generated when no arguments is passed to .logs #475

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Fix the URL generated when no arguments is passed to .logs #475

merged 3 commits into from
Mar 11, 2019

Conversation

AMR-KELEG
Copy link

Fixes #474
Remove the excess .log added to the end of the URL when .logs is
used with no arguments.

Fixes #474
Remove the excess .log added to the end of the URL when .logs is
used with no arguments.
@AMR-KELEG
Copy link
Author

I am sure that this tweak will fix the bug but I wasn't able to test it locally.
Do you have any recommendations on how to edit the default.py file such that I can test the bot?

@sushain97
Copy link
Member

Hmm, it's been a while since I've run this bot so I'm not sure. Are you getting an error when running it?

Also, I suggest just adding a test that verifies this behavior! That should be sufficient.

@AMR-KELEG
Copy link
Author

I am not sure what should happen when I run the bot locally.
I believe that the bot tries somehow to connect to the IRC channel.

My question is how to update values like host, channels in the default.py file?

nick = 'phenny'
host = ''
port = 6667
ssl = False
# This requires password to be set
sasl = False
ipv6 = False
channels = ['#example', '#test']

@sushain97
Copy link
Member

Ah, use a test channel like #amr-keleg-phenny-test. Host is probably the freenode IRC url. The others shouldn't need to be set.

self.input.group = lambda x: [None, None][x]
apertium_wiki.logs(self.phenny, self.input)
out = self.phenny.say.call_args[0][0]
string_check = ("Log at " in out) and (not out.endswith(".log"))
Copy link
Member

Choose a reason for hiding this comment

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

Please use single quotes to match the rest of the file. Also, just go ahead and inline this since there's only one check.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, the file has mixed single and double quotes.

Copy link
Member

Choose a reason for hiding this comment

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

Argh. Okay, then this is fine. If you'd like, switch them all to be consistent.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I would like to do so.
Does Apertium has some sort of coding conventions?
I also prefer single quotes for strings

Copy link
Member

Choose a reason for hiding this comment

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

For this repo, no. In apertium-streamparser, apertium-init and apertium-apy, we stick to single quotes. So, that works for me.

Copy link
Author

Choose a reason for hiding this comment

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

I prefer opening a new issue for the mixed single and double quotes.
I also think we should check the rest of the repo's files.

Copy link
Member

Choose a reason for hiding this comment

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

There are too many stylistic issues/inconsistencies for a single issue and given that this repo is a fork, I'd like to stay away from sweeping changes, It's fine for this file since this module doesn't exist in upstream.

@sushain97
Copy link
Member

Do you mind providing a transcript of a chat log while you were testing? Then this should be mergeable. Thanks!

@AMR-KELEG
Copy link
Author

Do you mind providing a transcript of a chat log while you were testing? Then this should be mergeable. Thanks!

Can I just copy the messages from the testing channel and paste them as a comment here?

@sushain97
Copy link
Member

Yeah, that's what I mean by transcript.

@AMR-KELEG
Copy link
Author

Do you mind providing a transcript of a chat log while you were testing? Then this should be mergeable. Thanks!

[00:12] == AMR-KELEG [***] has joined #amr-keleg-phenny-test
[00:12] == mode/#amr-keleg-phenny-test [+ns] by herbert.freenode.net
[00:12] == phenny [~phenny@***] has joined #amr-keleg-phenny-test
[00:12] <@AMR-KELEG> .log
[00:12] <phenny> Log at https://tinodidriksen.com/pisg/freenode/logs/%23amr-keleg-phenny-test/

@sushain97 sushain97 merged commit 9d0f610 into apertium:master Mar 11, 2019
@sushain97
Copy link
Member

Good work!

@AMR-KELEG
Copy link
Author

Thanks @sushain97.
I would like to know what will happen now.
Is there a server on which phenny is running?
And if this is the case, Will we need to restart the bot so that the changes will get applied?

@sushain97
Copy link
Member

@jonorthwash usually takes care of restarting it.

@jonorthwash
Copy link
Member

I pulled and restarted, but it looks like I only got the following updates:

Merge made by the 'recursive' strategy.
 modules/apertium_wiki.py           | 2 +-
 modules/test/test_apertium_wiki.py | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

@jonorthwash
Copy link
Member

Oh, interesting, those look like what the PR contained. I'd guessed the logs had their own module.

@AMR-KELEG
Copy link
Author

@jonorthwash Yes, actually the solution required a simple tweak.
Thanks.

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.

3 participants