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

Pass analyze #819

Merged
merged 5 commits into from Nov 5, 2015

Conversation

Projects
None yet
2 participants
@richafrank
Member

richafrank commented Nov 5, 2015

@elmq0022 , Since #818 was facing conflicts and such, I rebased and fixed up your branch. If this looks right to you, and tests pass, we can merge this one in.

puppy and others added some commits Nov 1, 2015

BUG: Issue #801 Initializing TradingAlgorithm Object Does Not Set
_analyze Method in algorithm.py

Added one line to check for the keyword argument 'analyze' and set the
the _analyze method when a TradingAlgorithm object is initialized
within a script.
@elmq0022

This comment has been minimized.

elmq0022 commented Nov 5, 2015

It looks great and all the test passed.

I assume the merge has to be done on your side, correct? Do you want me to
go back and close the issue #801 and pull request #818?

Also, this is my first contribution to an open source project. Honestly it
has been more of a learning curve than I expected. Thank you for helping
me get over the hump.

On Thu, Nov 5, 2015 at 9:23 AM, Richard Frank notifications@github.com
wrote:

@elmq0022 https://github.com/elmq0022 , Since #818
#818 was facing conflicts and
such, I rebased and fixed up your branch. If this looks right to you, and

tests pass, we can merge this one in.

You can view, comment on, or merge this pull request online at:

#819
Commit Summary

  • BUG: Issue #801 Initializing TradingAlgorithm Object Does Not Set
  • BUG: Address issue #801 and add test. Pass panel directly to
  • TST: Removed extra parameters
  • STY: Fixed typo and appeased flake8
  • TST: Fixed up test result comparison

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#819.

@richafrank

This comment has been minimized.

Member

richafrank commented Nov 5, 2015

It looks great and all the test passed.

Great!

I assume the merge has to be done on your side, correct? Do you want me to go back and close the issue #801 and pull request #818?

That's correct. I can update everything on merging.

Also, this is my first contribution to an open source project. Honestly it has been more of a learning curve than I expected. Thank you for helping me get over the hump.

And we really appreciate it! Sorry there's such a curve, but I'm happy to help! And we're always looking for ways to improve the experience...

richafrank added a commit that referenced this pull request Nov 5, 2015

@richafrank richafrank merged commit 375f2ef into master Nov 5, 2015

2 checks passed

Scrutinizer 6 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@richafrank richafrank deleted the pass_analyze branch Nov 5, 2015

@elmq0022

This comment has been minimized.

elmq0022 commented Nov 5, 2015

Nah, wasn't that bad. The just more issues than I was expecting, mostly
related to my environment. There seems to be some dependencies beyond what
the list in the development guide: SQLAlchemy, parameterized-nose, toolz,
and cython to name a few.

If I can get these all resolved and actually run the test suite and build
the docs, things will go better. In fact, getting this done and updating
the developer guide may be a good next task.

On Thu, Nov 5, 2015 at 10:30 AM, Richard Frank notifications@github.com
wrote:

It looks great and all the test passed.

Great!

I assume the merge has to be done on your side, correct? Do you want me to
go back and close the issue #801
#801 and pull request #818
#818?

That's correct. I can update everything on merging.

Also, this is my first contribution to an open source project. Honestly it
has been more of a learning curve than I expected. Thank you for helping me
get over the hump.

And we really appreciate it! Sorry there's such a curve, but I'm happy to
help! And we're always looking for ways to improve the experience...


Reply to this email directly or view it on GitHub
#819 (comment).

@richafrank

This comment has been minimized.

Member

richafrank commented Nov 5, 2015

Oh interesting! I think those dependencies should be in requirements.txt or requirements_dev.txt, but definitely let us know if anything is missing or submit an update the guide.

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