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

Parsing incorrectly grouping nodes #4

Closed
vmuriart opened this Issue Dec 20, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@vmuriart
Copy link

vmuriart commented Dec 20, 2017

  • SmallCalc version: 0.1.0
  • Python version: 3.5
  • Operating System: Any

Description

When reviewing the parsing tree on the tests one of them looked like it was wrongly grouping the left and right tokens for operations.

For the example being test 2 + 3 - 4 it doesn't matter first operation is a +. But if the example becomes 2 - 3 + 4 then the AST indicates that 3 and 4 should be added up first before applying the subtraction. The example below might explain it better.

What I Did

$ python cli.py
smallcalc :> 10 - 1 + 1

Result should be (10, 'integer')
instead returns (8, 'integer')

@vmuriart

This comment has been minimized.

Copy link
Author

vmuriart commented Dec 20, 2017

The AST representation to better explain this from the article you linked in an earlier post

AST

I think the correct test then would be the one below (though, i wrote it manually, so it may be wrong)

def test_parse_expression_with_multiple_operations():
    p = cpar.CalcParser()
    p.lexer.load("2 + 3 - 4")

    node = p.parse_expression()

    assert node.asdict() == {
        'type': 'binary',
        'left': {
            'type': 'binary',
            'left': {
                'type': 'integer',
                'value': 2
            },
            'right': {
                'type': 'integer',
                'value': 3
            },
            'operator': {
                'type': 'literal',
                'value': '+'
            }
        },
        'right': {
            'type': 'integer',
            'value': 4
        },
        'operator': {
            'type': 'literal',
            'value': '-'
        }
    }
@lgiordani

This comment has been minimized.

Copy link
Owner

lgiordani commented Dec 23, 2017

Well, thanks. You found a big issue! Actually I wonder how I could overlook this :shame:
The AST is reversed, as you noticed, so the minus operator is applied to the whole subsequent expression. I'll fix this, but since I have also to fix the posts it will take some time. Thanks a lot for spotting this!!

@lgiordani

This comment has been minimized.

Copy link
Owner

lgiordani commented Dec 24, 2017

I fixed the code and the posts. Hopefully I changed everything that needed to be changed. I run again through the 3 posts to check everything. Thanks again for the issue. Keep following the series and pointing out errors ;)

@lgiordani lgiordani closed this Dec 24, 2017

@vmuriart

This comment has been minimized.

Copy link
Author

vmuriart commented Dec 26, 2017

No worries. Your posts have definitely been a help with the project I'm working on (especially to use as an example of a TDD project).

One question about TDD i was hoping to answer through this issue though was, does TDD have guidelines on how to deal in the case that a test was found to be wrong retroactively?

Part of the drive for TDD is that since tests are developed independently of the code is that the tests are "correct" and thus in (most cases) tests shouldn't be changed to accommodate the code that was developed later.
In this case, it was found that the test was fundamentally incorrect, so there is no choice but to update the tests and the code to fix those tests, but I feel like this breaks something in the TDD process.

My question, in simpler terms, is:
For TDD, tests shouldn't be updated to conform to the code, but rather the code should be changed to make the tests work. How does this balance to the case where the test itself is incorrect and where does this line get drawn?
An example would be when a PR is submitted with new code & changed to the tests because the tests are now wrong because of the changes the PR did. Often I've argued that the code should be updated so that the new feature gets implemented w.o. changing and of the tests (other than adding new ones).

@lgiordani lgiordani reopened this Dec 27, 2017

@lgiordani

This comment has been minimized.

Copy link
Owner

lgiordani commented Dec 27, 2017

Oh, right. Well as you say in TDD you shouldn't change the tests to conform to the code, but tests have to be maintained as code has to be. If at a certain point during the development you want to change how some part of the system works you have to change tests to reflect the new behaviour. You have to change tests FIRST, which is what you correctly address when you say that "the code should be changed to make the tests work".

Sometimes, if the change is radical, you need to remove all the tests of a certain part and start from scratch. The limiting case is when you decide to change some fundamental choice like the language you used. In that case you really need to start from scratch (perhaps with a completely different paradigm).

In this case, alas, I have a series of posts that depend on that code, and as I wanted to introduce people to AST building and other compiler techniques, I didn't want to keep some wrong code. I considered it, but it's not a good practice to tell a reader that what they are reading is wrong 😄

In a standard TDD-driven project (like I usually have) I would have fixed the tests, or even introduced some new ones, and then I would have changed the code accordingly. This is definitely the way to go.

The PR case that you talk about is clear at this point, I think. If you added some features you should just add new tests for the code. Well, actually your process should be the opposite, but the final result is that you added code and tests. If your PR changes something, however, yes the tests are wrong and shall be replaced. Don't mistake the process for the outcome, however. Your process, as the PR author, is to add new tests and remove the old ones in a TDD way, that is, possibly one test at a time. It is not always possible to work this way when you refactor, because when you change parts of the code you may be invalidating more than one test.

Refactoring with tests is easy if your project is well-structured and you can isolate components. In that case you can invalidate groups of tests being sure of exactly what you are removing or leaving untested. I tried to show an example of refactoring with tests in one of my posts, but in that case the code had no tests at all.

I hope this answers your question, feel free to open a new issue if there are still dark corners in the matter, maybe there is enough stuff for a clarifying post. Thanks

@lgiordani lgiordani closed this Dec 27, 2017

@vmuriart

This comment has been minimized.

Copy link
Author

vmuriart commented Dec 27, 2017

Agreed that the case above should be fixed early on the tutorial, otherwise readers might end up confused as to why their calculator isn't working correctly if they followed everything upto the 3rd post (assuming the 4th post would be the one introducing the fix).

Thanks for elaborating on the points above. It's one of the aspects about TDD I struggle with a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.