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

Allow using BatchRequest for Pulls #90

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

vietle-bh
Copy link
Contributor

Issues addressed by this PR

Closes #89

The change allowed me to use BatchRequest for pulling from multiple worksheets without reopening the workbook.

I'm a complete newbie in this repo 😅but hope this is good enough for a merge!

Test files

TestBatchPulling.zip

Changelog

Additional comments

@vietle-bh vietle-bh added the type:feature New capability or enhancement label Feb 11, 2025
@vietle-bh vietle-bh self-assigned this Feb 11, 2025
@vietle-bh vietle-bh requested a review from adecler February 11, 2025 15:44
Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

That's definitely a step in the right direction. The only thing that bother me is that the 'SelectMany' removes the boundary between the different requests. So you don't really know which output belongs to which request:
image

@adecler
Copy link
Member

adecler commented Feb 12, 2025

@vietle-bh , I've done a quick commit to solve the problem highlighted above:
image

Not sure if the 'OriginalRequest' property was really necessary but I thought it was better to be safe than sorry.
Let me know what you think. Feel free to edit if necessary.

@vietle-bh
Copy link
Contributor Author

Not sure if the 'OriginalRequest' property was really necessary but I thought it was better to be safe than sorry. Let me know what you think. Feel free to edit if necessary.

Thanks @adecler ! That 'OriginalRequest' property is actually perfect. I previously had to include the worksheet name in each Excel tab in order to divide the flat result from batch Pull back into sublists, one per original request. It's much easier to use now after your addition 🙌

@vietle-bh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check core

Copy link

bhombot-ci bot commented Feb 12, 2025

@vietle-bh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check core

There are 4 requests in the queue ahead of you.

@vietle-bh vietle-bh requested a review from adecler February 13, 2025 09:02
Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

Works as intended. Now that the output is clustered by request, I am happy with the code.

@adecler
Copy link
Member

adecler commented Feb 13, 2025

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Feb 13, 2025

@adecler to confirm, the following actions are now queued:

  • check ready-to-merge

There are 1 requests in the queue ahead of you.

@adecler adecler merged commit 9f7b295 into develop Feb 13, 2025
12 checks passed
@adecler adecler deleted the Excel_Toolkit-#89-EnableBatchPull branch February 13, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow pulling from multiple worksheets
2 participants