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

Fix wrapper cast_to_a1_notation #1427

Merged
merged 3 commits into from
Mar 2, 2024
Merged

Fix wrapper cast_to_a1_notation #1427

merged 3 commits into from
Mar 2, 2024

Conversation

lavigne958
Copy link
Collaborator

fix the wrapper method cast_to_a1_notation.

It must check that the 4 first arguments are actual ints.

Then it should only try to cast the arguments at position 3 and 4 only, not the rest of the list.

closes #1055

fix the wrapper method cast_to_a1_notation.

It must check that the 4 first arguments are actual ints.

Then it should only try to cast the arguments at position
3 and 4 only, not the rest of the list.

closes #1055

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958 lavigne958 self-assigned this Feb 27, 2024
@alifeee
Copy link
Collaborator

alifeee commented Feb 28, 2024

If there were bugs before, and this fixes the bugs. We should have a test that proves it. :)

@lavigne958
Copy link
Collaborator Author

If there were bugs before, and this fixes the bugs. We should have a test that proves it. :)

You're right I'll add a test. I'll add a call to the method define_named_range with both a1 and index based coordinates.

The detail is: it's a bug only on methods with no kwargs and multiple args with no default values. So its quiet specific.

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@alifeee
Copy link
Collaborator

alifeee commented Feb 28, 2024

You're right I'll add a test. I'll add a call to the method define_named_range with both a1 and index based coordinates.

I am confused about the test. You added a single test that calls self.sheet.define_named_range(1, 1, 2, 2, range_name). This uses index-based coordinates.

Where is the "a1 [...] based coordinate"?

@lavigne958
Copy link
Collaborator Author

You're right I'll add a test. I'll add a call to the method define_named_range with both a1 and index based coordinates.

I am confused about the test. You added a single test that calls self.sheet.define_named_range(1, 1, 2, 2, range_name). This uses index-based coordinates.

Where is the "a1 [...] based coordinate"?

I noticed we already have a test that calls this method with the a1 notation. So I added the one with coordinates.

It makes more sense right to have them both in the test, I'll add it.

@lavigne958
Copy link
Collaborator Author

I looked around and found just above a test that adds a named range with A1 coordinates, so I merged the 2 tests into a single one that then deletes the named range then creates the exact same range with index based coordinates. and ensures everything is created the correct way.

@lavigne958 lavigne958 merged commit 3c4fd5f into master Mar 2, 2024
10 checks passed
@lavigne958 lavigne958 deleted the bugfix/fix_cast_to_a1 branch March 2, 2024 10:48
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.

cast_to_a1_notation(method) bugs?
2 participants