Skip to content

Conversation

@zverok
Copy link
Collaborator

@zverok zverok commented May 25, 2016

As per our email discussion.
What is done here:

  • redesign of all the specs according to modern RSpec style (subject/let, small contexts, shared examples);
  • arit_spec.rb was merged into basic_spec.rb;
  • added #inspect to Basic class, as per Add meaningful #inspect to classes #40, for better debugging:
    • before: expected: #<SymEngine::Pow:0xa84e298>, got: #<SymEngine::Add:0xa84e2d4>
    • after: expected: #<SymEngine::Pow(x**y)>, got: #<SymEngine::Add(z)>)
  • cleanup of spec_helper.rb and adding new rspec-its gem for shorter matchers.

Hope that helps.

@certik
Copy link
Contributor

certik commented May 26, 2016

@zverok thanks a lot for helping us redesign it!

There are some test failures, are those introduced by this PR? If so, then they need to be fixed.

@zverok
Copy link
Collaborator Author

zverok commented May 26, 2016

@certik Oops, I was busy yesterday evening, checked specs only locally (they worked), forgot to re-check at Travis. Will fix tiday.

@abinashmeher999
Copy link
Contributor

@zverok Thanks for taking the time to redesign it!

The specs are shorter and more readable now. In many cases things that you have added, I didn't even know one could do it that way.

As you pointed it out, we need to expose shorter functions for Klass.new methods or do something about the name clashes.

.travis.yml Outdated
- gem install gem-path --no-ri --no-rdoc
- cd $HOME/gems/symengine-0.1.0/
- bundle exec rspec
- bundle exec rspec --require ./spec/spec_helper.rb
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we are testing against installed gem, and gem files are not including .rspec, we need to specify this directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Since we are already including the spec/ folder, I guess there is no harm in including .rspec too. By the way, should spec/ folder be packaged with the gem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm usually including it :)
And, if we'll look at popular gems, they are typically doing the same thing.
So, I think it is ok (as well as .rspec file inclusion).

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. We can do it after this PR is merged. Let's wait for the tests.

@zverok
Copy link
Collaborator Author

zverok commented May 26, 2016

OK, Travis seems happy with current state of the things, and AppVeyor is somehow hanging it for ages. I think, it can be merged (it is really doubtful that specs redesign have broke something only on Windows).

@isuruf isuruf merged commit 98fb192 into symengine:master May 27, 2016
@isuruf
Copy link
Member

isuruf commented May 27, 2016

Thanks for the PR

@abinashmeher999
Copy link
Contributor

Thanks @zverok for the PR!

@zverok
Copy link
Collaborator Author

zverok commented May 27, 2016

👍

@zverok zverok deleted the rspec-style branch May 27, 2016 06:26
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.

4 participants