narwhals-dev / narwhals

Lightweight and extensible compatibility layer between dataframe libraries!
https://narwhals-dev.github.io/narwhals/
MIT License
384 stars 51 forks source link

test: remove `to_list()` and `to_numpy()` from Series tests #804

Closed MarcoGorelli closed 3 weeks ago

MarcoGorelli commented 3 weeks ago

Let's try to write all tests using compare_dict from tests.utils

The reason is that to_list forces all data into a list of Python objects, which isn't ideal:

If we just use compare_dicts across tests, then having a single source of determining whether result matches expected should help us out in the long-run

So, the task here is, when a test uses to_list, to rewrite it so that we're comparing dictionaries. See https://github.com/narwhals-dev/narwhals/pull/801 for several examples of how to do this

A couple of examples of functions that still need changing:

There's a few more

contributions welcome! if you're new, please read the CONTRIBUTING.md file, and choose one file at a time. no need to ask for permission to work on this

specialkapa commented 3 weeks ago

Hi @MarcoGorelli. Good to e-meet you. I have done some work to replace to_list and use compare_dicts instead. Tests seem to be passing. Followed the instruction on CONTRIBUTING.md but I cannot seem to push my local branch to the remote repository. Note that there is no remote branch that I am tracking. Rather I am trying to push using the command below:

git push --set-upstream origin test/804/remove-to-list-and-to-numpy.

Am I missing something obvious? Should I create the remote branch and track it with the local one and then push?

Thanks

MarcoGorelli commented 3 weeks ago

git push --set-upstream origin test/804/remove-to-list-and-to-numpy.

yup, this looks fine 👍

specialkapa commented 3 weeks ago

Thanks for getting back to me. Upon executing the push command above I am prompted to type my user name and access token.

But I get this error:

remote: Permission to narwhals-dev/narwhals.git denied to specialkapa. fatal: unable to access 'https://github.com/narwhals-dev/narwhals.git/': The requested URL returned error: 403

Apologies in case it is something obvious. I am used to using SSH rather than credentials. Any idea what that issue is?

MarcoGorelli commented 3 weeks ago

hey, I'd suggest working through https://github.com/firstcontributions/first-contributions before you try contributing

specialkapa commented 3 weeks ago

roger! thank you :)

luke396 commented 3 weeks ago

I'm also working on some code, but I'll push or merge it after @specialkapa fixes his environment. Welcome to our new contributor!

MarcoGorelli commented 3 weeks ago

thanks @luke396 ! it's unlikely that you worked on exactly the same files, and there'll be other issues for @specialkapa to work on, I'd suggest just pushing your code for now

specialkapa commented 3 weeks ago

hi @MarcoGorelli, silly me used the https clone URL rather than the SSH one. I am confident I am satisfying all instructions in https://github.com/firstcontributions/first-contributions but I seem to not have write access. Here is the feedback I get on my terminal:

_ERROR: Permission to narwhals-dev/narwhals.git denied to specialkapa. fatal: Could not read from remote repository.

Please make sure you have the correct access rights and the repository exists._

FBruzzesi commented 3 weeks ago

Hey @specialkapa, unless you have some privilege in a repo, you should work on a fork by creating a dedicated branch there, and then open a PR from the fork to the main repo.

Otherwise you will not have the privileges you need to perform some actions. Feel free to reach out on our discord or to me personally on discord if you need help with any of these steps.

I hope this helps 😇

specialkapa commented 3 weeks ago

Ah thanks for this. I don't think I came across this requirements in the CONTRIBUTING.md. Is this something that I missed or this something that is implied and I should have known?

FBruzzesi commented 3 weeks ago

Thanks for the feedback! You are right that none of what I have mentioned is explicitly stated in the guidelines we currently have. We will add some of these necessary preliminary steps for more clarity👌

MarcoGorelli commented 3 weeks ago

I think it's also mentioned there: https://github.com/firstcontributions/first-contributions?tab=readme-ov-file#fork-this-repository

specialkapa commented 3 weeks ago

@MarcoGorelli you're right. Thanks for pointing this out.