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

Testing auto-gen docs for DataFrame #230

Merged
merged 23 commits into from
Apr 21, 2020
Merged

Testing auto-gen docs for DataFrame #230

merged 23 commits into from
Apr 21, 2020

Conversation

DivvyCr
Copy link
Contributor

@DivvyCr DivvyCr commented Apr 18, 2020

  • added optional clean flag to analyze_replay_file(). Default is True.

self.file_path = file_path
self.load_json(loaded_json)
Copy link
Member

Choose a reason for hiding this comment

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

Is this just moving everything into a different function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - It makes it much easier to read, imo?
I also split up the properties into multiple functions for the same purpose.. All the validity checks made it really hard to get the idea of what's happening

Copy link
Member

Choose a reason for hiding this comment

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

after you get the tests working you should check the performance to make sure it is not massively slower.

Copy link
Contributor Author

@DivvyCr DivvyCr Apr 18, 2020

Choose a reason for hiding this comment

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

I've never heard of split functions hindering performance - it just passes parameters, and python has low overhead for making method calls (as well as most other langs)

carball/tests/docs/df_methods.txt Outdated Show resolved Hide resolved
carball/tests/docs/df_summary.txt Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Build carball 0.6.43-ffsooeoo failed (commit 13849cbbe3 by @DivvyC)

1 similar comment
@AppVeyorBot
Copy link

Build carball 0.6.43-ffsooeoo failed (commit 13849cbbe3 by @DivvyC)

These methods should not be visible to the external user.
@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request introduces 1 alert and fixes 1 when merging c2aa2f2 into b5a673e - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #230 into master will increase coverage by 1.16%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   89.83%   91.00%   +1.16%     
==========================================
  Files         100      100              
  Lines        4555     4567      +12     
  Branches      778      782       +4     
==========================================
+ Hits         4092     4156      +64     
+ Misses        335      281      -54     
- Partials      128      130       +2     

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 1 alert when merging c090fa3 into b5a673e - view on LGTM.com

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

dtracers
dtracers previously approved these changes Apr 19, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 1 alert when merging 51ce723 into b5a673e - view on LGTM.com

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 1 alert when merging 98eaf9a into b5a673e - view on LGTM.com

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

Build carball 0.6.44-bqfhgodi completed (commit ba1a9f947e by @DivvyC)

1 similar comment
@AppVeyorBot
Copy link

Build carball 0.6.44-bqfhgodi completed (commit ba1a9f947e by @DivvyC)

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 1 alert when merging 55f3a98 into b5a673e - view on LGTM.com

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2020

This pull request fixes 1 alert when merging 05f2a93 into f08c711 - view on LGTM.com

fixed alerts:

  • 1 for Wrong number of arguments in a call

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

@dtracers dtracers merged commit f8515c3 into SaltieRL:master Apr 21, 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