Ruby wrapper for parser #60

Open
wants to merge 7 commits into
from

Projects

None yet

5 participants

@rajithv
Contributor
rajithv commented Jul 8, 2016

No description provided.

@rajithv rajithv Ruby wrapper for parser
588fa76
@isuruf
Member
isuruf commented Jul 8, 2016

Can you add some tests?

rajithv added some commits Jul 8, 2016
@rajithv rajithv Update symengine version
cf77487
@rajithv rajithv testing parse method
300c769
@rajithv rajithv Merge branch 'master' of https://github.com/symengine/symengine.rb in…
…to parser
5980a4a
@zverok zverok commented on the diff Jul 8, 2016
spec/symengine_spec.rb
@@ -18,5 +18,12 @@
it { is_expected.to be_a SymEngine::Rational }
its(:to_s) { is_expected.to eq '1/3' }
end
+
+ describe 'parse' do
+ subject { SymEngine::parse('123 + 321') }
+
+ it { is_expected.to be_a SymEngine::Integer }
+ it { is_expected.to eq 444 }
@zverok
zverok Jul 8, 2016 Collaborator

What will be if there's erroneous/unparseable input? (And, BTW, what input, if any is considered erroneous or unparseable by symengine?)

@rajithv
rajithv Jul 8, 2016 Contributor

@zverok I did think about that. In C++ code it will throw errors for unparseable code. Checking that may almost be equivalent to doing the parsing. There are 4 errors,

  1. Expected token!
  2. Invalid symbol or number!
  3. Mismatching parantheses!
  4. Operator inconsistency!

Catching the exceptions from the C++ code and throwing ruby exceptions will be covered in the weeks 9 and 10. So this issue will be solved then.

As of now, I wanted the tests to merely confirm that the Ruby wrapper is communicating successfully with the cwrapper. Actually the same idea was behind the matrix wrapper tests.

@zverok
zverok Jul 8, 2016 Collaborator

Got it. I just always became suspicious when I see tests for success and not tests for fail :)
In fact, I always prefer to add empty contexts for such a deferred tests pathes, like this:

context "When there is parsing error" do
  # TODO: will be covered on weeks 9-10
end

Though, I'm not insisting, currently.

@rajithv
rajithv Jul 8, 2016 Contributor

That's a quite nice way to make it clear!

I'll keep this as it is, because week 9 is just one week away from now, probably I will start working on it even earlier.

@rajithv
Contributor
rajithv commented Jul 31, 2016

@isuruf can we merge this now, that the symengine/symengine#1044 has ParseError, so exception catching regarding Parser can be done in #63 with other exceptions?

@isuruf
Member
isuruf commented Aug 21, 2016

Can you resolve conflicts and add tests for parser errors?

rajithv added some commits Aug 21, 2016
@rajithv rajithv Merge branch 'master' of https://github.com/symengine/symengine.rb in…
…to parser
a2f88e2
@rajithv rajithv Exception Handling and test with parse error
c20e7a0
@isuruf isuruf commented on the diff Aug 21, 2016
spec/symengine_spec.rb
@@ -18,5 +18,16 @@
it { is_expected.to be_a SymEngine::Rational }
its(:to_s) { is_expected.to eq '1/3' }
end
+
+ describe 'parse' do
+ subject { SymEngine::parse('123 + 321') }
@isuruf
isuruf Aug 21, 2016 Member

Since these specs act as documentation, can you add more strings for parsing?
eg.

  • x + (y * 3 - 5)
  • x ** y + f(x)
  • sin(x) + cos(y) / 2
  • 1 / 2 + 3 / 2
  • 1 + 2 * 2
  • tan(0) - sqrt(2)
@abinashmeher999
abinashmeher999 Aug 22, 2016 Contributor

I would like to add a few more to this list

  • 5+-3
  • 5--3

All Without spaces
And

  • 5++3 or something like this to raise an error.
@rajithv rajithv Added more tests for parser
7e9ca53
@rajithv
Contributor
rajithv commented Aug 22, 2016

@isuruf @abinashmeher999 please review.

@abinashmeher999 abinashmeher999 commented on the diff Aug 22, 2016
spec/symengine_spec.rb
@@ -18,5 +18,52 @@
it { is_expected.to be_a SymEngine::Rational }
its(:to_s) { is_expected.to eq '1/3' }
end
+
+ describe 'parse' do
+ subject { SymEngine::parse('123 + 321') }
+
+ it { is_expected.to be_a SymEngine::Integer }
+ it { is_expected.to eq 444 }
+ end
+
+ it 'gives parse errors' do
+ expect { SymEngine::parse('12a + n34a9') }.to raise_error(RuntimeError)
@abinashmeher999
abinashmeher999 Aug 22, 2016 Contributor

IMO the error message should be checked too since we don't have specific exception for ParseError.

@certik
certik Aug 22, 2016 Contributor

Why not to add ParseError as a specific exception? That seems worthwhile.

@abinashmeher999
abinashmeher999 Aug 22, 2016 Contributor

Found just the thing that explains creating custom exceptions from Ruby C API: http://clalance.blogspot.in/2011/01/writing-ruby-extensions-in-c-part-5.html

@isuruf
isuruf Aug 24, 2016 Member

You don't have to create new exceptions.

  • SYMENGINE_RUNTIME_ERROR - rb_eRuntimeError
  • SYMENGINE_DIV_BY_ZERO - rb_eZeroDivError
  • SYMENGINE_NOT_IMPLEMENTED - rb_eNotImpError
  • SYMENGINE_PARSE_ERROR - rb_eSyntaxError
  • SYMENGINE_UNDEFINED - rb_eFloatDomainError ?? (I'm not sure what SYMENGINE_UNDEFINED is. Is this supposed to be a Domain error?)
@abinashmeher999
abinashmeher999 Aug 24, 2016 Contributor

I partially agree to the above. While RuntimeError and ZeroDivisionError should be used, our code atleast shouldn't raise NotImplementedError and SyntaxError. The latter ones are ScriptErrors and not StandardErrors.
This post explains why it would be abuse of NotImplementedError. SyntaxError is not a right fit since it is raised while encountering invalid Ruby code. If the user wants to handle a parse error, (s)he will have to handle SyntaxError and that would also lead to unwanted silencing of the actual syntax error in the source code.

Also there is the technical problem that a rescue clause by default only catches the StandardErrors. It is highly advised that custom application level exceptions be derived from StandardError.

For SYMENGINE_UNDEFINED it can be just StandardError.

@isuruf
isuruf Aug 24, 2016 edited Member

I just checked SciRuby/nmatrix repo and they use NotImplementedError and IOError (instead of SyntaxError)

@rajithv
Contributor
rajithv commented Aug 24, 2016

@certik @abinashmeher999 @isuruf should I make another PR to continue this work after the GSoC period? Because the submission guidelines asked not to change the submitted material after submission. Pls advice on this.

@abinashmeher999
Contributor

I assume the submission guidelines mention about putting in the exact amount of work that was achieved during the GSoC period. That you have mentioned in the blog post already. As long as you are not changing the contents and the amount of work achieved, you won't be violating any rules. Correct me If I am wrong.

You can keep the new exception making part for another PR. I think we can go ahead with merging this. Just a few more changes that have been suggested and we will be good to go.

@isuruf
Member
isuruf commented Oct 8, 2016

@rajithv, do you have time to finish this PR? If not, can you enable edits from maintainers?

@isuruf isuruf added this to the Release 0.1.0 milestone Oct 8, 2016
@certik
Contributor
certik commented Oct 11, 2016

@rajithv ping.

@rajithv
Contributor
rajithv commented Oct 12, 2016

@isuruf @certik I will find sometime to complete this PR

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