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

PEP 508: Grammar fixes #851

Closed

Conversation

doismellburning
Copy link

mindw and others added 2 commits December 10, 2018 15:48
Running the test without the fix results in:
```
>python p508.py
Traceback (most recent call last):
  File "p508.py", line 163, in <module>
    parsed = compiled(test).specification()
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\parsley.py", line 85, in invokeRule
    ret, err = self._grammar.apply(name, *args)
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 462, in apply
    val, err = self._apply(r, ruleName, args)
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 495, in _apply
    [rule(), self.input])
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 1079, in rule_specification
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 598, in _or
    ret, err = f()
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 1071, in _G_or_376
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 495, in _apply
    [rule(), self.input])
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 1006, in rule_url_req
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 495, in _apply
    [rule(), self.input])
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 859, in rule_name
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 495, in _apply
    [rule(), self.input])
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 850, in rule_identifier
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 676, in consumedby
    _, e = expr()
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 847, in _G_consumedby_294
  File "c:\Users\gabi\_envs\extra_test\lib\site-packages\ometa\runtime.py", line 554, in many
    v, _ = fn()
  File "/pymeta_generated_code/pymeta_grammar__Grammar.py", line 844, in _G_many_296
AttributeError: 'Grammar' object has no attribute 'rule_identifier_end'
```
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

doismellburning added a commit to doismellburning/pep508 that referenced this pull request Dec 10, 2018
@doismellburning
Copy link
Author

Is there any precedent/plan for automated testing of "PEP code"? Should there be?

I've taken the liberty of throwing together https://github.com/doismellburning/pep508 because I kept picking up slightly buggy grammar variants but I can't imagine this is the first time something subtle has slipped through...

@doismellburning
Copy link
Author

(I've signed the CLA, just waiting for it to be received and processed)

@doismellburning
Copy link
Author

@mindw Would you be able and willing to sign the CLA please?

| marker_and:m -> m
marker = marker_or
quoted_marker = ';' wsp* marker
identifier_end = letterOrDigit | (('-' | '_' | '.' )* letterOrDigit)
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this fix without changing spacing in all lines? (for a better history, annotate, etc)

Copy link
Author

@doismellburning doismellburning Dec 22, 2018

Choose a reason for hiding this comment

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

I'm no fan of vertical alignment for similar reasons, but I was aiming to remain consistent with the author's style

I feel like if I were to PR a fix just to this line, that would be the worst-of-both-worlds - everything vertically aligned apart from one line - and that either this PR (retain alignment) or a two-commit approach (the first to eliminate alignment, the second to fix the bug) are both better options - which would you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

Doing a second commit has the same issues (misleading history etc).

It also wouldn’t work on Python repos as PRs use squash and merge.

@brettcannon
Copy link
Member

Thanks for the PR, but the CLA has still not been signed and this PR now conflicts with the repo, so I'm closing this.

@brettcannon brettcannon closed this Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants