We can see from the coverage report that they're not being run:
https://app.codecov.io/gh/pandas-dev/pandas/blob/main/pandas/tests/io/json/test_readlines.py
This is because their name doesn't start with test_
So, task here is:
- rename the tests so they start with test_
- run them, check if they pass
- stage, commit, push, pull request
Comment From: MarcoGorelli
cc @SFuller4 just FYI
looks like the last two don't actually pass
Comment From: MarcoGorelli
we probably need a pre-commit check for this https://github.com/pandas-dev/pandas/issues/50379
Comment From: phershbe
@MarcoGorelli I can do this if you'd like. I haven't worked as a programmer yet and am relatively new to open source, but this is easy enough, it's the last four tests I guess.
Comment From: MarcoGorelli
that's right
Comment From: MarcoGorelli
same this one
https://github.com/pandas-dev/pandas/blob/3b60759002fdc13edbbb1eab6c4bc5ec14d027ef/pandas/tests/series/methods/test_explode.py#L76-L81
Comment From: phershbe
@MarcoGorelli Okay, great, thank you. I'm working on it and having trouble running the tests. This is a really important project that moves really fast so I didn't self-assign it with take because if anybody else jumps in and does it that's fine, I'm working on it though.
Comment From: MarcoGorelli
no hurry 😄
I'm working on it and having trouble running the tests
what trouble are you having? feel free to ask, either here or on the slack (see the readme for the link)
Comment From: labibdotc
take
Comment From: phershbe
@MarcoGorelli Awesome, thank you for mentioning the slack, I forgot about that. I'm starting now, I'll ask in a short time here if I'm still stuck.
Comment From: phershbe
@MarcoGorelli The issue got taken, I'm still working on it for learning purposes and can submit it if the other person doesn't get it done. I'm looking at some other issues that you've opened too. Thank you.
Comment From: MarcoGorelli
Feel free to submit - @labibdotc please don't comment "take" when someone else has clearly expressed interest in working on an issue
Comment From: labibdotc
I commented a take with the assumption no one has it assigned from the header. Will go over comments next time. @phershbe sorry for the hassle, take it now.
Comment From: phershbe
take
Comment From: phershbe
@labibdotc No problem. I also should have taken after expressing interest in order to avoid something like this.
Comment From: MarcoGorelli
no worries, thanks both!
Comment From: MarcoGorelli
Please make sure to remove this line
https://github.com/pandas-dev/pandas/blob/1b653b1c23f7a5f9fbaede993cf9370dba79b1fc/.pre-commit-config.yaml#L345
when you open your pull request
@labibdotc if you're interested, https://github.com/pandas-dev/pandas/issues/50380 is similar (though slightly harder)
Comment From: phershbe
@MarcoGorelli Working on it now, have to get the two tests to pass like you mentioned before, if I get stuck I'll ask in a short time here.
Comment From: phershbe
@MarcoGorelli Okay, I was working on it and got it mostly figured out. The True and False on line 388 and again on line 417 (https://github.com/pandas-dev/pandas/blob/main/pandas/tests/io/json/test_readlines.py) are getting turned into 1.0 and 0.0 when they go through ensure_clean and to_json and read_json. The tests pass by changing the booleans to numbers, but it seems like there should be a cleaner solution. Of course it could be done by making them the numbers and then commenting the code.
I'm learning a lot. I have some holiday stuff to do now and tomorrow so I may not be able to get it done until Monday morning.
Comment From: phershbe
@MarcoGorelli Okay, it's because JSON turns booleans into 1s and 0s, probably that's pretty common knowledge but I don't have much experience so it took me a while. The best idea I've found so far is to put int in front of the booleans (int(True) and int(False)) in both cases where we define the expected variable so that assert_frame_equal can compare the two sets of data equally. I'm going to submit the pull request like that and then feel free to change it if you know of a better way.