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

Update the zap template used for chip-tool to not forced the first le… #6930

Merged

Conversation

vivien-apple
Copy link
Contributor

@vivien-apple vivien-apple commented May 18, 2021

…tter of arguments to be lower cased

Problem

Followup to #6873 where arguments such as CSRNonce ends up as cSRNonce.

Summary of Changes

  • Update the template so it does not enforce the first letter to be lowercased

Testing

  • I have runned ./scripts/tests/test_suites.sh just to validate that argument parsing is not broken

@@ -186,9 +186,9 @@ public:
{
{{#chip_server_cluster_command_arguments}}
{{#if (isString type)}}
AddArgument("{{asCamelCased label}}", &m{{asCamelCased label false}});
AddArgument("{{asCamelCased label false}}", &m{{asCamelCased label false}});
Copy link
Contributor

Choose a reason for hiding this comment

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

For future PRs we may want to look into API readability: the false argument is not obvious in purpose. Maybe something like startUpperCamelCase and startLowerCamelCase methods would be more readable, although I find those namings awkward as well. Can these be chained? like camelCase label | uppercaseFirstLetter

Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

Per the updated template, can you update the PR here?

#### Problem
What is being fixed?  Examples:
* Fix crash on startup
* Fixes #12345 12345 Frobnozzle is leaky (exactly like that, so GitHub will auto-close the issue).

#### Change overview
What's in this PR

#### Testing
How was this tested? (at least one bullet point required)
    • If unit tests were added, how do they cover this issue?
    • If unit tests existed, how were they fixed/modified to prevent this in future?
    • If integration tests were added, how do they verify this change?
    • If manually tested, what platforms controller and device platforms were manually tested, and how?
    • If no testing is required, why not?

@stale
Copy link

stale bot commented May 28, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label May 28, 2021
@stale
Copy link

stale bot commented Jun 4, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Jun 4, 2021
@bzbarsky-apple bzbarsky-apple reopened this Jun 4, 2021
@stale stale bot removed the stale Stale issue or PR label Jun 4, 2021
@bzbarsky-apple
Copy link
Contributor

@vivien-apple This is still relevant, right?

@stale
Copy link

stale bot commented Jun 11, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Jun 11, 2021
@woody-apple
Copy link
Contributor

/rebase

@stale stale bot removed the stale Stale issue or PR label Jun 16, 2021
@woody-apple
Copy link
Contributor

/rebase

@stale
Copy link

stale bot commented Jun 24, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Jun 24, 2021
@vivien-apple vivien-apple force-pushed the ChipTool_ArgumentLowercase branch from da739ac to 2f18ca6 Compare June 25, 2021 16:36
@stale stale bot removed the stale Stale issue or PR label Jun 25, 2021
@vivien-apple
Copy link
Contributor Author

@woody-apple I have finally updated the template...

@woody-apple woody-apple merged commit c662feb into project-chip:master Jun 25, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
project-chip#6930)

* Update the zap template used for chip-tool to not forced the first letter of arguments to be lower cased

* Update gen/ folders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants