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

WASM ABI: iter_start -> datastore_table_scan_bsatn #1637

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 26, 2024

Description of Changes

Rename iter_start -> datastore_table_scan_bsatn.

@Centril Centril added abi-break A PR that makes an ABI breaking change release-0.12 labels Aug 26, 2024
@Centril Centril force-pushed the centril/datastore-table-scan-bsatn branch from e31db25 to 0e2807a Compare August 26, 2024 19:28
/// - `NOT_IN_TRANSACTION`, when called outside of a transaction.
/// - `NO_SUCH_TABLE`, when `table_id` is not a known ID of a table.
// #[tracing::instrument(skip_all)]
pub fn datastore_table_scan_bsatn(
Copy link
Contributor

Choose a reason for hiding this comment

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

Those diffs really throw me off tbh - not much of a problem when it's 1 line, but harder to compare when it's an entire function.

Could be nice to have them in the same position just to quickly verify that functionality didn't change, only names did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be nice to have them in the same position just to quickly verify that functionality didn't change, only names did.

(I'm intentionally moving these to be in the same order as the proposal.)

Copy link
Contributor

@RReverser RReverser Aug 27, 2024

Choose a reason for hiding this comment

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

Yeah you mentioned that on PRs with smaller changes, just saying it makes reviews (and, I imagine, Git blame) somewhat harder.

Is the order matching the proposal really that important?

@Centril Centril force-pushed the centril/datastore-table-row-count branch from d76b397 to 7c3a4a7 Compare August 27, 2024 22:49
@Centril Centril force-pushed the centril/datastore-table-scan-bsatn branch from 0e2807a to 5c37b10 Compare August 27, 2024 23:17
@Centril Centril changed the base branch from centril/datastore-table-row-count to master August 27, 2024 23:17
@Centril Centril enabled auto-merge August 27, 2024 23:18
@Centril Centril added this pull request to the merge queue Sep 2, 2024
Merged via the queue into master with commit 49f366f Sep 2, 2024
8 checks passed
@Centril Centril deleted the centril/datastore-table-scan-bsatn branch September 2, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change release-0.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants