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

PR: Support for all Pandas indexes in Variable Explorer #5149

Merged
merged 4 commits into from
Sep 11, 2017

Conversation

Prikers
Copy link
Contributor

@Prikers Prikers commented Sep 6, 2017

Fixes #3758


Hi team,

This is a proposal in order to solve #3758.
In addition to the support of pandas DatetimeIndex (#3849) in the Variable Explorer it is possible to support all pandas Indexes.
Results are exactly the same as before for DatetimeIndex and very similar for all other types (RangeIndex, MultiIndex, CategoricalIndex, etc.).
capture1
capture2

@pep8speaks
Copy link

pep8speaks commented Sep 6, 2017

Hello @Prikers! Thanks for updating the PR.

Line 78:1: E302 expected 2 blank lines, found 1
Line 88:1: E302 expected 2 blank lines, found 1
Line 92:80: E501 line too long (96 > 79 characters)

Line 50:1: E302 expected 2 blank lines, found 1
Line 234:1: E302 expected 2 blank lines, found 1

Comment last updated on September 09, 2017 at 11:15 Hours UTC

@ccordoba12 ccordoba12 added this to the v4.0beta1 milestone Sep 7, 2017
@ccordoba12
Copy link
Member

Wow!! This is an amazing addition!! Thanks @Prikers! I hope you've found navigating through our sourcecode not so hard :-)

Don't worry about the style issues reported by pep8speaks in our test files. We can ignore those.

@dalthviz, please review this one.

@ccordoba12 ccordoba12 requested a review from dalthviz September 7, 2017 21:33
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Nice work @Prikers ! 👍, I left some minor comments about the tests but they are simple suggestions, I think the functionality by itself is ok :)

@@ -17,6 +17,7 @@
import os

# Third party imports
import pandas
Copy link
Member

Choose a reason for hiding this comment

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

Maybe could you import the classes for the indexes in the next line and not import all pandas?

""" Creates a dictionnary of many possible pandas indexes """
return {
'Index': pandas.Index(list('ABCDEFGHIJKLMNOPQRST')),
'RangeIndex': pandas.RangeIndex(0, 20),
Copy link
Member

Choose a reason for hiding this comment

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

If the import change is done then you should delete the pandas. to call the indexes classes here.


def test_dataframeeditor_with_various_indexes():
for rng_name, rng in generate_pandas_indexes().items():
rng = rng[:3]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of taking only the first 3 elements you could take all the elements and then assert the first three and the last one.

@Prikers
Copy link
Contributor Author

Prikers commented Sep 9, 2017

Thank you for the kind words @ccordoba12 and @dalthviz! Indeed it takes me some time to navigate through the code 😆 but that helps me learning a lot!
@dalthviz I took your comments into account, it should be ready now.
I also removed the pep8 issue that were legitimate. I did not consider the remaining pep8 issues because it would break the style unity of the code.

@ccordoba12 ccordoba12 changed the title PR: Support for all pandas indexes in Variable Explorer PR: Support for all Pandas indexes in Variable Explorer Sep 11, 2017
@ccordoba12 ccordoba12 merged commit 91dc6ee into spyder-ide:master Sep 11, 2017
@Prikers Prikers deleted the pandasIndexes branch September 23, 2017 11:17
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