-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add public current depth field to JSON reader. #1802
Conversation
/// The depth of the current token. This read-only field tracks the recursive depth of the nested | ||
/// objects or arrays within the JSON text processed so far, and it shouldn't be modified by the | ||
/// caller. | ||
int32_t current_depth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffreyRichter, do you think it makes more sense to expose this field from the az_json_token
struct rather than from the az_json_reader
? I have it from the reader because it is easier to access from the caller (the user doesn't have to fish it out from the nested token field), but the depth can be thought of as a property of a particular JSON token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share the customer code that uses this field. The usage patterns would probably highlight if it should be on the reader on the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did above in the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I didn't want to add it to the token is, right now, the token can be created as a standalone type, and used outside of the reader (at least when looking at the public fields). Adding depth to it turns it into something that has to be a part of a larger JSON payload (and then we might not be able to use it in the future for other cases like in the writer).
Also, in .NET, I had added the depth on the reader directly (but we didn't have the nested token struct there) :)
The reason why I posed the question is because I feel that if we emphasize single responsibility, a case can easily be made to move the field to az_json_token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that said, this is still in beta and we can revisit it separately, if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I'd prefer in the json reader rather than the token unless there's a good reason for it to be in the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will close on it later, and decide if a change is needed or we are happy where we are:
#1808
@RickWinter or @antkmsft any other feedback or thoughts before I merge this? |
* Sync eng/common directory with azure-sdk-tools for PR 1579 (#1724) * Allow propogation of SetDevVersion. * Allow use of SetDevVersion locally and across stages. Co-authored-by: Mitch Denny <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1524 (#1728) * Make RELEASE_TITLE_REGEX more specific * Update eng/common/scripts/ChangeLog-Operations.ps1 Co-authored-by: Wes Haggard <[email protected]> * Update release REGEX * tighten up changelog verification * Ensure releaseDate is in the appriopriate format. * Update error messages. * Retrun false after error Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Wes Haggard <[email protected]> * Update samples to include log of mqtt client username (#1731) * Typo "Github"→"GitHub" (#1738) * Revert changes to SetDevVersion. (#1740) Co-authored-by: Mitch Denny <[email protected]> * Add better error handling in Prepare-Release (#1746) Co-authored-by: Wes Haggard <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1594 (#1747) * Update sparse checkout create directory step name * plumb BaseBranchName to create pull request Co-authored-by: Ben Broderick Phillips <[email protected]> * Fix sample readme urls (#1752) * allow multiple invocations of archetype-sdk-tests-generate (#1754) Co-authored-by: scbedd <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1585 (#1758) * Sparse checkout docs repository in release step * Detect default branch for custom sparse checkout repository * Use sparse checkout paths parameter in docs metadata release Co-authored-by: Ben Broderick Phillips <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1615 (#1761) * Move docindex.yml from steps -> jobs * Add docindex.yml Co-authored-by: Daniel Jurek <[email protected]> * add clang format version (#1755) * Sync eng/common directory with azure-sdk-tools for PR 1627 (#1763) * Refactor sparse checkout template: idempotency and no yaml loops * Add logging, re-organize sparse checkout script Co-authored-by: Ben Broderick Phillips <[email protected]> * Add Get-ChangeLogEntriesFromContent (#1764) Co-authored-by: Chidozie Ononiwu <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1635 (#1767) * Update common engineering system for docs onboarding * Add updates from review feedback * Space nit Co-authored-by: Daniel Jurek <[email protected]> * add empty section for compile options (#1741) * Enable Caching of PS Modules (#1769) - Remove copied AzPowershell utilities - Add latest AZ module path already on hosted agents to PSModulePath - Rename setup-az-modules template setup-environments to reflect what is is doing - Add support for Caching the current user PS Module folder - Add support for install-module if not already present in module folder - Organize the live test clean-up script to be in the standard location Co-authored-by: Wes Haggard <[email protected]> * Adding new CA trusted certificates (#1765) Adding new CA certificates and updating the documentation to create a store when testing in Windows or OSX. * Sync eng/common directory with azure-sdk-tools for PR 1611 (#1766) * Add API status check * Revert temp change * Update as per review comments * Removed blank line * Review comments to use az cli * Changes to move az cli commands to caller script and other review comments * Skip languages that's not supported in APIView * Fix bug in language mapping Co-authored-by: praveenkuttappan <[email protected]> Co-authored-by: praveenkuttappan <[email protected]> * Fix for Issue #1768 - Response Types (#1770) * Add the ability to check for open pull request to a different repo. (#1774) Co-authored-by: Chidozie Ononiwu <[email protected]> * Fetch specific branch name only in git-branch-push script (#1771) Co-authored-by: Ben Broderick Phillips <[email protected]> * Use generate matrix job name parameter as display name (#1776) Co-authored-by: Ben Broderick Phillips <[email protected]> * Updating documentation for ECC server certificate chain support. (#1773) * Sync eng/common directory with azure-sdk-tools for PR 1633 (#1778) * Update format of new Changelog Entry * Add parsing of changelog sections * Update ChangeLog Logic Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> * Strict mode needs the variables initialized (#1779) Co-authored-by: Wes Haggard <[email protected]> * Fix of depricated BearSSL method call of WiFiClientSecure class in sample for ESP8266. (#1780) * Fix of depricated BearSSL method call in sample for ESP8266. * Fix of depricated BearSSL method call of WiFiClientSecure class in sample for ESP8266. Co-authored-by: ts\bozicekm <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1688 (#1782) * Add verification of changelog sections * Update eng/common/scripts/ChangeLog-Operations.ps1 Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Wes Haggard <[email protected]> * Update for master -> main branch rename (#1783) * ci.yml -- Add forward compatability with "main" to triggering branches * Fix hard-coded master (#1788) Co-authored-by: Chidozie Ononiwu <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1716 (#1790) * Update links with master to use main * Update ci.yml files Co-authored-by: Chidozie Ononiwu <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1719 (#1793) * Update pipeline generation tool version Consume latest changes from pipeline generation tool in Azure/azure-sdk-tools#1708 * Update tool version to include fix for public ci Co-authored-by: Wes Haggard <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1729 (#1794) * Check for API review status only if release date is set in changelog * Change property name to ReleaseStatus Co-authored-by: Praveen Kuttappan <[email protected]> * Add Ubuntu 20 to local dns bypass template (#1795) Co-authored-by: Praveen Kuttappan <[email protected]> Co-authored-by: praveenkuttappan <[email protected]> * Update master to main (#1798) * Update references from master to main (#1799) Co-authored-by: Wes Haggard <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1725 (#1805) * Bring changes from JS docs metadata * Move business logic inside Update-DocsMsMetadata.ps1 * Update with the latest changes in JS PR * Update from latest PR feedback * Add check for empty path to prevent crashes when creating relative paths Co-authored-by: Daniel Jurek <[email protected]> * Update devops workitem helpers (#1806) - Switch to using rest instead of cli for querying work items as cli is limited to 1000 items only. - Fix issues with Generated fields not being set. - Correctly sort older workitems by version isntead of string. Co-authored-by: Wes Haggard <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1763 (#1807) * Disable release date check * Release check is not finding release date Co-authored-by: Praveen Kuttappan <[email protected]> Co-authored-by: praveenkuttappan <[email protected]> * only add compiler flags for tests (#1791) * Update docs metadata mutation logic (#1810) Co-authored-by: Daniel Jurek <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1772 (#1812) * Update change log headers based on guideline update Updates based on Azure/azure-sdk#3103 - Renamed "Key Bugs Fixed" to "Bugs Fixed" - Renamed "Fixed" to "Other Changes" Added a warning in validation if at lease one of the recommended headers aren't used. * Update eng/common/scripts/ChangeLog-Operations.ps1 Co-authored-by: JoshLove-msft <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: JoshLove-msft <[email protected]> * Add public current depth field to JSON reader. (#1802) * Add public current depth field to JSON reader. * Add a CL entry. * Fix typo in src comment. * Update codeowners file (#1809) * Switch ubuntu 18 to ubuntu 20 (#1814) Co-authored-by: Chidozie Ononiwu <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 1767 (#1813) * Support building and deploying bicep templates * Add bicep powershell install aka link to deployment error message * Write bicep compiled arm templates to temp directory * Simplify bicep building code/function usage * Use bicep location for compiled arm templates, and remove them on success Co-authored-by: Ben Broderick Phillips <[email protected]> * Add base64 encoding and decoding APIs that accept az_span. (#1816) * Add base64 encoding and decoding APIs that accept az_span. * Add entry to the changelog. * Address PR feedback - move const to r.h.s. * Fix link substitution (#1820) * Fix doc publishing to support basic link substitution * Remove publishing for az_storage_blobs Co-authored-by: Azure SDK Bot <[email protected]> Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Chidozie Ononiwu <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: Mollie Munoz <[email protected]> Co-authored-by: Hiroshi Yoshioka <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: Ben Broderick Phillips <[email protected]> Co-authored-by: scbedd <[email protected]> Co-authored-by: Daniel Jurek <[email protected]> Co-authored-by: Dane Walton <[email protected]> Co-authored-by: Cristian Pop <[email protected]> Co-authored-by: praveenkuttappan <[email protected]> Co-authored-by: praveenkuttappan <[email protected]> Co-authored-by: Miha Božiček <[email protected]> Co-authored-by: ts\bozicekm <[email protected]> Co-authored-by: Praveen Kuttappan <[email protected]> Co-authored-by: JoshLove-msft <[email protected]> Co-authored-by: Rick Winter <[email protected]>
Fixes #1744
The following snippet can now be re-written using
public
APIs:azure-sdk-for-c/sdk/src/azure/iot/az_iot_hub_client_properties.c
Line 242 in 75afd98
As follows:
Note when the depth is incremented:
cc @danewalton