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

refactor: rename Resource.allocated to Resource.allocate #3516

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

tKe
Copy link
Contributor

@tKe tKe commented Oct 25, 2024

No description provided.

@tKe
Copy link
Contributor Author

tKe commented Oct 25, 2024

I'm not sure if the js test failure is anything to do with this change

@serras
Copy link
Member

serras commented Oct 28, 2024

The 1.2.4 branch shows that at that point there were two versions: allocated and allocate, with the latter being "safer". It's true that the signature correspond to the previous allocate, so this refactoring makes sense.

@nomisRev do you remember the history of this change? Do you think it makes sense to go back to allocate, as the signature matches 1.2.4?

@serras serras requested a review from nomisRev October 29, 2024 07:12
@kyay10
Copy link
Collaborator

kyay10 commented Nov 2, 2024

Using a verb makes more sense because it executes the Resource

Copy link
Collaborator

@kyay10 kyay10 left a comment

Choose a reason for hiding this comment

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

Is there a source compatibility guarantee for this? Probably not because it was declared delicate.

@tKe
Copy link
Contributor Author

tKe commented Nov 2, 2024

The current 1.2.4 release has both allocated and allocate with the former deprecated and the latter being recommended. So this change would actually make arrow 2.0 more source compatible.

@kyay10
Copy link
Collaborator

kyay10 commented Nov 2, 2024

Oh, so this actually brings source compatibility! Wonderful! I'll wait on Simon to have a look, but I'm all for this change!

@serras serras merged commit 129206f into arrow-kt:main Nov 5, 2024
10 checks passed
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.

3 participants