Micro-optimisations: Delivery API node parse improvement - #23506
Conversation
|
Hi there @patrickdemooij9, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
AndyButland
left a comment
There was a problem hiding this comment.
Thanks again @patrickdemooij9, also looks good. I've pushed a few more targeted tests, which did lead to me seeing a minor regression in functionality. It's only one for a malformed request, but to be 100% sure I've ensured the behaviour hasn't changed.
I've also run some automated smoke tests with the delivery API to ensure the expansion behaviour works as before.
|
Thanks for taking the time to look at them @AndyButland . Do you write the tests yourself or do you generate them? If so, is there a standard prompt I might be able to use? Would save you time on my pull requests next time |
|
I'm generating and then reviewing them. Nothing very sophisticated - just working with Claude and asking it to verify coverage of the functionality that's changed, and where it's found lacking, generate unit tests and verify the pass both before and after the refactoring. |
* Delivery API Node parse improvement * PR feedback from myself * Further minor optimisation in ElementOnlyOutputExpansionStrategy. * Add tests and revert a minor change to previous behaviour. --------- Co-authored-by: Andy Butland <abutland73@gmail.com>
Prerequisites
Description
The Node.Parse currently goes through each char in a string and checks if there are certain characters. Because we already know the subset of characters that we want to match on, we can just jump to the characters that we need and then resolve everything before that to reduce allocations.
I ran two benchmarks. The impact is not that big, but it gets bigger with the amount of text included.
Value = $all
Value = properties[header,footerGrid,footerLegal,pageTitles]