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

Implement __eq__ method for Cell #1063

Merged
merged 8 commits into from
Jun 17, 2022
Merged

Conversation

chisvi
Copy link
Contributor

@chisvi chisvi commented Jun 15, 2022

As suggested in #1061, the code included in this PR implements the __eq__ method for Cell class.

This way, it will be possible to compare two Cell instances (cell_1 == cell_2).

Two cell instances are considered "equal" when they represent the cell at the same row and col and contain the same value.

This feature is particullary useful for testing purposes.

closes #1061

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

Hi thank you for this pr.

I notice you added a test but I don't see the test cassette. Did you record your test ? Does it run successfully?

I notice your PR does not pass the CI, you can follow the contribution guidelines (can be found at the bottom bof the readme of the repo) you'll find all you need to contribute and make your PR pass the CI

@chisvi
Copy link
Contributor Author

chisvi commented Jun 16, 2022

Hi thank you for this pr.

I notice you added a test but I don't see the test cassette. Did you record your test ? Does it run successfully?

I notice your PR does not pass the CI, you can follow the contribution guidelines (can be found at the bottom bof the readme of the repo) you'll find all you need to contribute and make your PR pass the CI

Hi @lavigne958! Yes, I've recorded my test but didn't provide the cassete, sorry. Regarding this I've a couple of questions:

  • Should I include in my PR the test cassete for all tests, or just the one that has been created for my new test?
  • Also I see that the cassete contains sensible info like the Bearer token used to authenticate the request, so I'm not sure how to proceed to avoid to push it.

Let me check the CI and fix whatever is broken (weird, since I ran all the checks locally and were ok).

@lavigne958
Copy link
Collaborator

Hi thank you for this pr.
I notice you added a test but I don't see the test cassette. Did you record your test ? Does it run successfully?
I notice your PR does not pass the CI, you can follow the contribution guidelines (can be found at the bottom bof the readme of the repo) you'll find all you need to contribute and make your PR pass the CI

Hi @lavigne958! Yes, I've recorded my test but didn't provide the cassete, sorry. Regarding this I've a couple of questions:

  • Should I include in my PR the test cassete for all tests, or just the one that has been created for my new test?
    yes you should, but regarding the situation, you added a new test, so you should only have a single new file for your new test.
    When recording what you can do is:
  1. discard any changes on your cassettes: git restore --stagged tests/cassettes/ then git restore tests/cassettes/
  2. start the test suite with the env variables set to record new episodes: GS_RECORD_MODE=new_episodes tox -e py
  3. then you should only have a single new file for your new test.
  • Also I see that the cassete contains sensible info like the Bearer token used to authenticate the request, so I'm not sure how to proceed to avoid to push it.
    yes but they are removed when recording the cassettes, see here

Let me check the CI and fix whatever is broken (weird, since I ran all the checks locally and were ok).

you can find a tox command that will format your code: tox -e format.

@chisvi
Copy link
Contributor Author

chisvi commented Jun 17, 2022

I've just added the cassette, thanks for your help!

Regarding the formatter, after running it with tox -e format it left the code in an status the linter was complaining about. I had to set some variable to avoid having multiline comparisons and therefore avoid linting issues.
imagen

gspread/cell.py Show resolved Hide resolved
gspread/cell.py Show resolved Hide resolved
@lavigne958
Copy link
Collaborator

I've just added the cassette, thanks for your help!

Regarding the formatter, after running it with tox -e format it left the code in an status the linter was complaining about. I had to set some variable to avoid having multiline comparisons and therefore avoid linting issues. imagen

Did you understand what the error message is about ? because the changes you made are not exactly what the linter is asking you to fix, but because you break it into multiple variables it fixes the warning from the linter (which is better in my opinion).

@lavigne958 lavigne958 merged commit 4c2e265 into burnash:master Jun 17, 2022
@chisvi
Copy link
Contributor Author

chisvi commented Jun 17, 2022

I've just added the cassette, thanks for your help!
Regarding the formatter, after running it with tox -e format it left the code in an status the linter was complaining about. I had to set some variable to avoid having multiline comparisons and therefore avoid linting issues. imagen

Did you understand what the error message is about ? because the changes you made are not exactly what the linter is asking you to fix, but because you break it into multiple variables it fixes the warning from the linter (which is better in my opinion).

I think so: it's complaining about putting a line break before binary operator (and). My first try was to put it after the binary operator, but then the linter was complaining about the format (which was putting it again before the binary operator), so I decided to change it to that final version.

@chisvi chisvi deleted the cell-comparison branch June 17, 2022 14:24
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.

Implement equality on Cell objects
2 participants