Skip to content

ENH: Add StataWriter 118 for unicode support #30285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 31, 2019
Merged

ENH: Add StataWriter 118 for unicode support #30285

merged 3 commits into from
Dec 31, 2019

Conversation

@bashtage
Copy link
Contributor

@bashtage bashtage commented Dec 16, 2019 •
edited
Loading

Add StataWriter with unicode support
jreback
jreback requested changes Dec 16, 2019
View reviewed changes
@simonjayhawkins simonjayhawkins added Enhancement IO Stata read_stata, to_stata labels Dec 16, 2019
jbrockmendel
jbrockmendel reviewed Dec 16, 2019
View reviewed changes
jbrockmendel
jbrockmendel reviewed Dec 16, 2019
View reviewed changes
@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 16, 2019

@kshedden can you double-check this? It looks fine to me, but I think you're better-informed on this than I am.
@kshedden
Copy link
Contributor

kshedden commented Dec 16, 2019

Sorry, I'm not familiar at all with the writers.
@pep8speaks
Copy link

pep8speaks commented Dec 16, 2019 •
edited
Loading

Hello @bashtage! 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! 🍻

Comment last updated at 2019-12-31 16:38:17 UTC
@bashtage bashtage force-pushed the stata-118-writer branch 3 times, most recently from e1d47c4 to 3cd1088 Compare December 17, 2019 00:05
WillAyd
WillAyd requested changes Dec 17, 2019
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

I think looks OK generally. Would have been easier to review if the refactor / f-strings were done separate
bashtage
bashtage commented Dec 17, 2019
View reviewed changes
Copy link
Contributor Author

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

I think looks OK generally. Would have been easier to review if the refactor / f-strings were done separate

I'm sure this is true. Just binged on them a bit.
@bashtage bashtage requested review from WillAyd and jreback December 17, 2019 21:13
@bashtage
Copy link
Contributor Author

bashtage commented Dec 17, 2019

scikit-learn related failure
jbrockmendel
jbrockmendel reviewed Dec 17, 2019
View reviewed changes
@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 17, 2019

LGTM. @TomAugspurger are we letting the sklearn thing in travis block merging?
@TomAugspurger
Copy link
Contributor

TomAugspurger commented Dec 17, 2019

Nope, it's OK to ignore.
@WillAyd WillAyd mentioned this pull request Dec 18, 2019
Closed
5 tasks
@jreback jreback added this to the 1.0 milestone Dec 26, 2019
jreback
jreback requested changes Dec 26, 2019
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. very minor comments, pls rebase and ping on green.
@bashtage
ENH: Add StataWriter 118 for unicode support
ea7e87c 
Add StataWriter with unicode support
@TomAugspurger
Copy link
Contributor

TomAugspurger commented Dec 30, 2019 •
edited
Loading

CI failure looked unrelated.

________________ TestRegistration.test_registering_no_warning _________________ [gw1] win32 -- Python 3.7.5 C:\Miniconda\envs\pandas-dev\python.exe self = <pandas.tests.plotting.test_converter.TestRegistration object at 0x000002411CB02788> def test_registering_no_warning(self): plt = pytest.importorskip("matplotlib.pyplot") s = Series(range(12), index=date_range("2017", periods=12)) _, ax = plt.subplots() # Set to the "warn" state, in case this isn't the first test run register_matplotlib_converters() with tm.assert_produces_warning(None) as w: > ax.plot(s.index, s.values)

Merged master to see if its fixed.
@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 31, 2019

CI failure again looks unrelated (and azure won't let me re-run). merged master to force re-run.
@bashtage
Copy link
Contributor Author

bashtage commented Dec 31, 2019

@jreback @jbrockmendel green.
@jbrockmendel jbrockmendel merged commit 35ba6b0 into pandas-dev:master Dec 31, 2019
@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 31, 2019

thanks @bashtage
hweecat pushed a commit to hweecat/pandas that referenced this pull request Jan 1, 2020
@bashtage @TomAugspurger
@jbrockmendel @hweecat
ENH: Add StataWriter 118 for unicode support (pandas-dev#30285)
a3ffa9c 
Add StataWriter with unicode support Co-authored-by: Tom Augspurger <TomAugspurger@users.noreply.github.com> Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
@bashtage bashtage deleted the stata-118-writer branch July 28, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement IO Stata read_stata, to_stata

8 participants

@bashtage @jbrockmendel @kshedden @pep8speaks @TomAugspurger @WillAyd @jreback @simonjayhawkins