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

#609 Prohibit Yada Operator #617

Closed
wants to merge 8 commits into
base: dev
from

Conversation

Projects
None yet
3 participants
@bentglasstube
Contributor

bentglasstube commented Jan 16, 2015

Submitting this as part of @neilbowers PR challenge.

I'm not really happy with the way the operator is detected versus the three dot range operator, but PPI didn't give me much to work with. If there is some problem with this PR please let me know and I will be happy to fix it.

@thaljef

This comment has been minimized.

Show comment
Hide comment
@thaljef

thaljef Jan 16, 2015

Member

Thanks for the fabulous patch. I left a few comments about specific lines on GitHub. If you can address those issues, I'd be happy to merge this. Thanks so much!

Member

thaljef commented Jan 16, 2015

Thanks for the fabulous patch. I left a few comments about specific lines on GitHub. If you can address those issues, I'd be happy to merge this. Thanks so much!

Show outdated Hide outdated Changes Outdated
@bentglasstube

This comment has been minimized.

Show comment
Hide comment
@bentglasstube

bentglasstube Jan 16, 2015

Contributor

Awesome, thanks for all the great feedback. I will fix up these issues
as soon as I have time. I may also try to tackle some of the other
issues on there now that I have some familiarity with the code.

On Thu, Jan 15, 2015 at 09:17:28PM -0800, Jeffrey Ryan Thalhammer wrote:

Thanks for the fabulous patch. I left a few comments about specific lines on GitHub. If you can address those issues, I'd be happy to merge this. Thanks so much!


Reply to this email directly or view it on GitHub:
#617 (comment)

Alan Berndt

Man is still the most extraordinary computer of all.
- John F. Kennedy

Contributor

bentglasstube commented Jan 16, 2015

Awesome, thanks for all the great feedback. I will fix up these issues
as soon as I have time. I may also try to tackle some of the other
issues on there now that I have some familiarity with the code.

On Thu, Jan 15, 2015 at 09:17:28PM -0800, Jeffrey Ryan Thalhammer wrote:

Thanks for the fabulous patch. I left a few comments about specific lines on GitHub. If you can address those issues, I'd be happy to merge this. Thanks so much!


Reply to this email directly or view it on GitHub:
#617 (comment)

Alan Berndt

Man is still the most extraordinary computer of all.
- John F. Kennedy

@moregan

This comment has been minimized.

Show comment
Hide comment
@moregan

moregan Jan 16, 2015

Contributor

PPI didn't give me much to work with

Can you elaborate on what you were hoping to get from PPI?

Contributor

moregan commented Jan 16, 2015

PPI didn't give me much to work with

Can you elaborate on what you were hoping to get from PPI?

@bentglasstube

This comment has been minimized.

Show comment
Hide comment
@bentglasstube

bentglasstube Jan 16, 2015

Contributor

I am not very familiar with PPI and I did not intend to slight it in any
way, so I hope my comment was not taken that way.

I was hoping for a way to differentiate between the three dot range
operator and the ellipses statement (herein referred to as the yada
operator).

From my experimentation, both appear as PPI::Token::Operator and so I
though to inspect the neighboring tokens to determine which I was
dealing with. I am not entirely satisfied with this approach.

If you are familiar with PPI and have a suggestion for a better way to
find the information I am looking for, I would be very interested to
hear your approach.

On Fri, Jan 16, 2015 at 09:55:42AM -0800, moregan wrote:

PPI didn't give me much to work with

Can you elaborate on what you were hoping to get from PPI?


Reply to this email directly or view it on GitHub:
#617 (comment)

Alan Berndt

There are no facts, only interpretations.
- Friedrich Nietzsche

Contributor

bentglasstube commented Jan 16, 2015

I am not very familiar with PPI and I did not intend to slight it in any
way, so I hope my comment was not taken that way.

I was hoping for a way to differentiate between the three dot range
operator and the ellipses statement (herein referred to as the yada
operator).

From my experimentation, both appear as PPI::Token::Operator and so I
though to inspect the neighboring tokens to determine which I was
dealing with. I am not entirely satisfied with this approach.

If you are familiar with PPI and have a suggestion for a better way to
find the information I am looking for, I would be very interested to
hear your approach.

On Fri, Jan 16, 2015 at 09:55:42AM -0800, moregan wrote:

PPI didn't give me much to work with

Can you elaborate on what you were hoping to get from PPI?


Reply to this email directly or view it on GitHub:
#617 (comment)

Alan Berndt

There are no facts, only interpretations.
- Friedrich Nietzsche

@moregan

This comment has been minimized.

Show comment
Hide comment
@moregan

moregan Jan 16, 2015

Contributor

I, too have thought that "yadda" should be distinct from the three-dot version of '..'. It's on my list to someday think about, but I don't know when I might get to it.

Contributor

moregan commented Jan 16, 2015

I, too have thought that "yadda" should be distinct from the three-dot version of '..'. It's on my list to someday think about, but I don't know when I might get to it.

bentglasstube added some commits Jan 18, 2015

Fix policy to allow non-numeric operands
It is possible to use the three dot range operator on things other than
numbers.  This solution is still a bit of a guess but it seems to work.
@bentglasstube

This comment has been minimized.

Show comment
Hide comment
@bentglasstube

bentglasstube Jan 18, 2015

Contributor

I think I have addressed all of your concerns now. Let me know if there is anything else.

Contributor

bentglasstube commented Jan 18, 2015

I think I have addressed all of your concerns now. Let me know if there is anything else.

@thaljef

This comment has been minimized.

Show comment
Hide comment
@thaljef

thaljef Aug 11, 2015

Member

This was merged into master at efcfed6 and will be released shortly. Sorry for the delay. Thanks for your contribution.

Member

thaljef commented Aug 11, 2015

This was merged into master at efcfed6 and will be released shortly. Sorry for the delay. Thanks for your contribution.

@thaljef thaljef closed this Aug 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment