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

Make get_values and alias of get #1296

Merged
merged 34 commits into from
Nov 5, 2023
Merged

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Sep 10, 2023

closes #1285, #1303

Having both of these method is ambiguous. I have made it so only get is the real method, and get_values is its alias.

def get_values(self, *args, **kwargs) -> ValueRange:
        """Alias for :meth:`~gspread.worksheet.Worksheet.get`"""
        return self.get(*args, **kwargs)

Potential problems:

  • get_values used to return type List[List[str]].
    get returns a ValueRange
    thus now get_values returns a ValueRange instead
    ValueRange subclasses list, so it's okay?

@alifeee alifeee added this to the 6.0.0 milestone Sep 10, 2023
@alifeee alifeee requested a review from lavigne958 September 10, 2023 16:05
@alifeee alifeee self-assigned this Sep 10, 2023
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.

I believe we can make it simpler, see comment.

Changing the outcome of the test is good sing of a breaking change 😮 .

We should add a new warning about it.

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

closes #1285

Having both of these method is ambiguous. I have made it so only get is the real method, and get_values is its alias.

def get_values(self, *args, **kwargs) -> ValueRange:
        """Alias for :meth:`~gspread.worksheet.Worksheet.get`"""
        return self.get(*args, **kwargs)

Potential problems:

  • get_values used to return type List[List[str]].
    get returns a ValueRange
    thus now get_values returns a ValueRange instead
    ValueRange subclasses list, so it's okay?

I agree about the duplicate method.

about the linting: I think we can solve it using the propose changes in the code.

@alifeee
Copy link
Collaborator Author

alifeee commented Sep 20, 2023

Changing the outcome of the test is good sing of a breaking change 😮 .

Yes, good that this is 6.0.0 ;). Do you think the breaking change is too big?

about the linting: I think we can solve it using the propose changes in the code.

Not sure what you mean by "linting"

assigning changed values back to response object
@lavigne958
Copy link
Collaborator

lavigne958 commented Oct 10, 2023

Changing the outcome of the test is good sing of a breaking change 😮 .

Yes, good that this is 6.0.0 ;). Do you think the breaking change is too big?

not too big, just may be it's too many. I don't know, I feel like this is a lot of changes.

about the linting: I think we can solve it using the propose changes in the code.

Not sure what you mean by "linting"

I meant typing sorry 🙊 but you solved it by applying the proposed code change

@alifeee
Copy link
Collaborator Author

alifeee commented Oct 17, 2023

Yes, this is a big change. It needs some thinking about.

The alternate options are:

  • leave get as-is, and make get_values be identical (alias) apart from it pads values. The names don't really make sense why this is the case, though.
  • try and make sure the API of get and get_values always remains the same

@alifeee
Copy link
Collaborator Author

alifeee commented Oct 19, 2023

For maximum compatibility, get_values should be an alias of get, but return type List[List[str]].

@alifeee alifeee marked this pull request as draft October 19, 2023 11:23
@alifeee
Copy link
Collaborator Author

alifeee commented Oct 19, 2023

I propose the breaking change here that:

  • get does start padding values

Then, the only difference between get and get_values is the return type (List[List[str]] vs ValueRange)

@lavigne958
Copy link
Collaborator

I propose the breaking change here that:

  • get does start padding values

I agree with this change, but the default behavior must stay so:

  • get can pad the values
  • when get is called by default it does not pad the values
  • when get_values calls get it requests it to pad the` values.

this way we keep the same behavior as before but we allow user to padd at both levels. Then we'll be able to deprecate the old behavior in the next+2 major release if we want to

Then, the only difference between get and get_values is the return type (List[List[str]] vs ValueRange)

can we add some flags again to offer the choice of the returned value ? default: keep the old return type, flag is set: return ValueRange ?

*I'm sorry I edited your comment instead of replying ! 🤦 *

@alifeee alifeee marked this pull request as ready for review October 20, 2023 09:28
@alifeee alifeee marked this pull request as draft October 24, 2023 22:20
@alifeee
Copy link
Collaborator Author

alifeee commented Oct 24, 2023

We are waiting for #1305 to be merged into feature/release_6_0_0 branch. Thus we are waiting for #1320.

@alifeee
Copy link
Collaborator Author

alifeee commented Oct 28, 2023

This is now ready to merge. Technically, this introduces no changes to the public API (👻wooo👻)

I have removed all code from worksheet.get_values so it is now simply

def get_values(self, *args, **kwargs) -> List[List[T]]:
"""Alias for :meth:`~gspread.worksheet.Worksheet.get`...
with ``return_type`` set to ``List[List[Any]]``
and ``pad_values`` set to ``True``
(legacy method)
"""
return self.get(
*args, pad_values=True, return_type=GridRangeType.ListOfLists, **kwargs
)

All heavy lifting is now done in the worksheet.get function.

You may notice that worksheet.get now has two very similar kwargs: pad_values and maintain_size. To see what the difference is, please see the readme

pad_values is introduced to differentiate the behaviour of get and get_values (see top of thread).

maintain_size is the new feature added by #1305

They cannot be combined into one kwarg for two reasons:

  • this would be a breaking change (we could do it here since this is for 6.0.0)
  • it would not be possible for indeteminate size ranges
    this is to say that pad_values operates on all arrays, just making them rectangular. maintain_size must know the size to pad the array to. Thus, you cannot use maintain_size for a call to "A:C", etc.

@alifeee alifeee requested a review from lavigne958 October 28, 2023 21:37
@alifeee alifeee marked this pull request as ready for review October 28, 2023 21:37
@alifeee
Copy link
Collaborator Author

alifeee commented Oct 31, 2023

added #1303 into this PR...

This was linked to issues Nov 1, 2023
tests/utils_test.py Show resolved Hide resolved
@alifeee alifeee merged commit 6944c1f into feature/release_6_0_0 Nov 5, 2023
10 checks passed
@alifeee alifeee deleted the alias/get_values branch November 5, 2023 12:14
@alifeee alifeee mentioned this pull request Jan 26, 2024
@alifeee alifeee mentioned this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for function arguments Why does get_values exist?
2 participants