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

Update the logic of solve method #6

Merged
merged 7 commits into from
Jan 19, 2018
Merged

Update the logic of solve method #6

merged 7 commits into from
Jan 19, 2018

Conversation

zhangt58
Copy link
Owner

To correctly handle the case of operations does not have returns.

- to get single variable value, e.g. 'pi', the new rpn is '1 pi *',
  rather than 'pi'.
@zhangt58 zhangt58 mentioned this pull request Jan 13, 2018
@zhangt58
Copy link
Owner Author

I'm not sure what is the output if you just type pi in elegant rpn terminal, not I changed it in pyrpn, that is:

pyrpn shell > 1 pi *
pyrpn shell > 3.1415926535900001
pyrpn shell > pi
Invalid RPN, fix it and try again.:)
pyrpn shell > 

@stevemolloy
Copy link
Contributor

In the elegant rpn shell:

CI0011774:~ stephenmolloy$ rpn
Welcome to rpn version 6, by Michael Borland and Robert Soliday (June 1999).
rpn> pi
  3.141592653589793

@stevemolloy
Copy link
Contributor

I just looked at the updated code. Very nice. It allows cleaner implementation of functions that don't return a value.
Will you merge this into master soon?

- do not push and pop operators into calculating stack (should speed up
  a little bit)
- add new sto function to store variable (temporarily in current Rpn
  instance)

Clean up docstring.
@zhangt58
Copy link
Owner Author

I've updated this PR, resolved two issues:

  1. Refactored solve logic to be much more clear, new function should be able to extended into Rpn very easily;
  2. sto operation supported, however, the pyrpn script should be upgraded.

The feature of supporting user-defined function in CLI is still pending, that is udf of elegant rpn.

Please help to check it.

@stevemolloy
Copy link
Contributor

I very much like this new structure. One worry I have is that I tend to dislike the use of getattr and hasattr methods, since they can sometimes lead to trouble -- imagine if a user-defined function shared the same name as a method of the Rpn class. But perhaps I am worrying too much :)

@zhangt58
Copy link
Owner Author

Good suggestion, now I make a dictionary to keep all the stored ones, pretty much simpler and clearer.

@zhangt58 zhangt58 merged commit fee706f into master Jan 19, 2018
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