issue_comments

My pleasure. Learnt a lot.

And it was a long-standing issue that was mentioned in a medium article about xarray looking for more contributors.

Thanks @aaronspring this is a great contribution.

Thanks again @aaronspring!

Hello @aaronspring! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-07-19 17:01:25 UTC

Thanks @aaronspring; this is looking very close! Just a few more minor suggestions.

I hope this is the final one. all tests pass. implemented your suggestions. took me a few commits, but I learned a lot. thanks for the guidance @spencerkclark

that's #4204. #4206 pinned the version of isort, so you should be able to get a green CI by merging master into your feature branch.

dont understand why isort fails

now aligns to display_width:

python for dw in [40,60,80,120]: with xr.set_options(display_width=dw): print(time[:2],dw,'\n')

``` CFTimeIndex([2000-01-01 00:00:00, 2000-01-02 00:00:00], dtype='object', length=2, calendar='gregorian') 40

CFTimeIndex([2000-01-01 00:00:00, 2000-01-02 00:00:00], dtype='object', length=2, calendar='gregorian') 60

CFTimeIndex([2000-01-01 00:00:00, 2000-01-02 00:00:00], dtype='object', length=2, calendar='gregorian') 80

CFTimeIndex([2000-01-01 00:00:00, 2000-01-02 00:00:00], dtype='object', length=2, calendar='gregorian') 120 ```

@spencerkclark I reimplemented the repr. Mostly went after the visuals. So now this looks like before but including calendar in the repr. Any more tests needed? Would you please give this a look?

I now reimplemented the visuals of pd.Index and also test against pd.Index.repr @spencerkclark

now after clicking on the data item:

this PR is now ready for review. I don't understand why azure tests are failing.

I found a workaround with to_index() in formatting.short_data_repr. this is makes one test fail: xarray/tests/test_dask.py TestDataArrayAndDataset.test_dataarray_repr. is this ok or not a valid way to implement a cftimeindex.repr?

EDIT: I ensure now that to_index is only done when if CFTimeIndex.

Ready for review.

Currently only the cftimeindex repr shows the calendar property. I aim to get it into the dataset/dataarray repr.

would it be a good idea to modify formatting.py.array_repr(array)? IndexVariable uses this repr. Once time is a coordinate, it becomes an IndexVariable. but the index variable doesnt have this cftime.calender property anymore. Should I try to add this calendar property to the array or would this not be desired?

I do understand the meaning of this last sentence @spencerkclark

My sense is that it is more important that indexes with the same date type have the same calendar attribute than that the calendar argument passed to cftime_range is propagated verbatim to the index it produces. What do you think?

Should I rather test on cftimeindex created in a different way?

This doesnt change the html repr yet: when clicking on the data symbol, calendar is not shown in the printout. I will further try to fix this

I was hoping to inherit from pandas like: python def __repr__(self): super().__repr__() return self.__repr__().strip(")")+f", calendar={self.calendar}')"

But I will now try to rebuild as in pandas.

Ah now I understand your questions. Thanks for clarifying.

My question was whether should replicate the pd.Index.repr or try to import or inherit as much as I can from pandas?

Maybe start by making the smallest possible change to the pandas repr?

EDIT: If we want to build our own repr, there are some helpful functions in core/formatting.py

how should I extend the __repr__ coming from pd.Index

if you define a __repr__ method under CFTimeIndex, won't it override pd.Index.__repr__?

Yes. I will do that. My question was whether should replicate the pd.Index.repr or try to import or inherit as much as I can from pandas?

how should I extend the __repr__ coming from pd.Index

if you define a __repr__ method under CFTimeIndex, won't it override pd.Index.__repr__?