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

Move ExcelCompiler functionality to Spreadsheet object while keeping gen_graph working with arguments #231

Closed
wants to merge 8 commits into from

Conversation

bradbase
Copy link

No description provided.

@bradbase
Copy link
Author

Python 3.4... gotta make that work.

@bradbase bradbase closed this Aug 17, 2019
@bradbase bradbase reopened this Aug 17, 2019
@danielsjf
Copy link
Collaborator

@bradbase This fix looks good if it fixes your issues. The Python 3.4 issues seems more of a glitch in the tests than a fundamental error. I've had it too in the past and it disappeared by itself. I'm not really sure what the origin is.

The move to the new ast is nice. The location of that code always bothered me :-)

Regarding the tests, there are a few things that disappear:

  • The creation of a spreadsheet from scratch but I don't care. This was mainly to add a new way of testing but it hasn't been used anywhere yet.
  • The spaces in sheet names. This was a bugfix and it might be good to check if we can restore this behaviour
  • A few renamings that I did, mainly in the spreadsheet file. val -> value; addr -> address. These are small fixes I suppose.

So if I understand it well, with this PR, there is no need to roll back the previous fixes?

@danielsjf
Copy link
Collaborator

@Brad-eki, @bradbase, if we can clear out the last remaining points we can merge this one. Since it is quite a big PR, it might be a shame to loose track due to future other PRs that modify similar parts. For instance #236 also interacts with astnodes might also need to be integrated in this one.

@bradbase
Copy link
Author

bradbase commented Mar 6, 2020

This approach can't work.

@bradbase bradbase closed this Mar 6, 2020
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