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

Resolve issue #297 #318

Merged
merged 4 commits into from Jun 1, 2018
Merged

Resolve issue #297 #318

merged 4 commits into from Jun 1, 2018

Conversation

sylints
Copy link
Contributor

@sylints sylints commented May 19, 2018

No description provided.

t/greppable.t Outdated
@@ -35,6 +35,9 @@ $t.test-gist(‘“…” is added to long paths’,
$t.test-gist(‘“…” is not added to root files’,
%(‘result.md’ => none /‘``…/README.md``’/));

$t.test(‘single line/module returned’,
“{$t.bot-nick}: 6lang”,
Copy link
Member

Choose a reason for hiding this comment

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

This is fine but fragile, I'd go with something like:

<AlexDaniel> greppable: ought to cover the same functionality as this class, maybe long-term we
<greppable6> AlexDaniel, 1 lines, 1 modules: https://gist.github.com/fafdd7e108cdd382ddf0d7a536f80385

This is pretty much guaranteed to always return one module.

bin/Greppable.p6 Outdated
@@ -105,8 +105,20 @@ multi method irc-to-me($msg) {
}
my $total = $stats.elems;
my $modules = $stats.Set.elems;
(‘’ but FileStore({ ‘result.md’ => $gist }))
but PrettyLink({“$total lines, $modules modules: $_”})
if $total == 1 and $modules == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

I somewhat dislike the copy-pasta here. What about:

but PrettyLink({{s $total, line}, {s $modules, module}: $_})

@AlexDaniel
Copy link
Member

Hello, any update on this? I'm very eager to merge this during the squashathon later today, but it would be nice to have the mentioned tweaks implemented.

@sylints
Copy link
Contributor Author

sylints commented Jun 1, 2018

My apologies but I have not had time to implement the changes you've suggested. Hopefully this can serve as LHF for new comers during this squashathon.

There's an “s” sub which is a quick hack, but it allows to temporarily
write things like this in a better way.
There's a high chance that “6lang” will be included in more than one
repository, so it's better to use some longer string.
Otherwise I'm seeing some infrequent failures locally.
@AlexDaniel AlexDaniel merged commit 726cfa9 into Raku:master Jun 1, 2018
@AlexDaniel
Copy link
Member

OK, did some tweaks to the PR and merged it. Thank you!

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

2 participants