[PM-25300] Add Sourcery usage on iOS architecture #631
Conversation
Deploying contributing-docs with
|
| Latest commit: |
e50d215
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6e764d9d.contributing-docs.pages.dev |
| Branch Preview URL: | https://ios-pm-23500-sourcery-usage.contributing-docs.pages.dev |
|
Great job! No new security vulnerabilities introduced in this pull request |
# Conflicts: # docs/architecture/mobile-clients/ios/index.md
|
Claude finished @fedemkr's task —— View job Review complete
SummaryThis PR adds clear and helpful documentation about Sourcery usage for automatic mock generation in the iOS project. The content is well-structured and includes practical examples. However, there are a few minor grammar and clarity issues that should be addressed. FindingsFinding 1: Grammar issues with possessive forms vs. noun adjuncts (lines 475, 486) Finding 2: Missing "to" in infinitive construction (line 476) Finding 3: Sentence structure could be improved for clarity (line 482) DetailsAll specific issues have been marked with inline comments at the relevant lines. The findings are minor grammatical improvements that will enhance readability. The changes to The addition of "Sourcery" to What was done wellThe documentation provides clear examples with code blocks and uses the appropriate Docusaurus info admonition for edge cases. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| ``` | ||
|
|
||
| :::info Manual generation |
There was a problem hiding this comment.
| :::info Manual generation | |
| or it cannot handle some closure types, especially in function's parameters. In such cases prefer |
| ``` | ||
|
|
||
| :::info Manual generation | ||
|
|
There was a problem hiding this comment.
| create the mock manually and remove the protocol's comment as `AutoMockable`. |
|
|
||
| #### Custom annotations | ||
|
|
||
| Sourcery allows us to annotate different parts of our code to guide code generation. On top of this |
There was a problem hiding this comment.
💭 Clarity: This sentence is somewhat awkward. Consider rephrasing for better readability.
Suggested rewrite:
| Sourcery allows us to annotate different parts of our code to guide code generation. On top of this | |
| Sourcery allows us to annotate different parts of our code to guide code generation. Custom annotations have been added in `AutoMockable.stencil` to handle special cases. |
| custom annotations have been added in `AutoMockable.stencil` to handle special cases. | ||
|
|
||
| - **useSelectorName**: Method annotation used to indicate that the generated mocked properties need | ||
| to use the selector name instead of the short method name. This is specially useful when using |
There was a problem hiding this comment.
| to use the selector name instead of the short method name. This is specially useful when using | |
| to use the selector name instead of the short method name. This is especially useful when using |
| [ios version](https://github.com/bitwarden/ios/blob/main/.test-simulator-ios-version)), otherwise | ||
| tests may fail because of subtle differences between iOS versions. | ||
|
|
||
| ### Mocks generation |
There was a problem hiding this comment.
🎨 Heading structure: Consider using a more descriptive heading like "Mock generation" instead of "Mocks generation" (singular form is more standard in English).
| :::info Manual generation | ||
|
|
||
| There are some cases where the automatically generated mock does not cover the mock scenario we want | ||
| or it cannot handle some closure types, especially in function's parameters. In such cases prefer to |
There was a problem hiding this comment.
⛏️ I would say "function parameters". If we still want to make it possessive instead of a noun adjunct, then it should be plural: "functions' parameters", but that sounds off to my ear.
|
|
||
| There are some cases where the automatically generated mock does not cover the mock scenario we want | ||
| or it cannot handle some closure types, especially in function's parameters. In such cases prefer to | ||
| create the mock manually and remove the protocol's comment as `AutoMockable`. |
There was a problem hiding this comment.
⛏️ I would say "The protocol's AutoMockable comment".
|
|
||
| - **useSelectorName**: Method annotation used to indicate that the generated mocked properties need | ||
| to use the selector name instead of the short method name. This is especially useful when using | ||
| function overloading where we need the mocked names to also have the parameters names to |
There was a problem hiding this comment.
⛏️ Either "parameter names" (noun adjunct, my preference) or "parameters' names" (possessive of plural noun)
| :::info Manual generation | ||
|
|
||
| There are some cases where the automatically generated mock does not cover the mock scenario we want | ||
| or it cannot handle some closure types, especially in function parameters. In such cases prefer to |
There was a problem hiding this comment.
⛏️ Grammar: "function parameters" (noun adjunct) is more natural than "function's parameters" (possessive).
| or it cannot handle some closure types, especially in function parameters. In such cases prefer to | |
| or it cannot handle some closure types, especially in function parameters. In such cases prefer to |
|
|
||
| There are some cases where the automatically generated mock does not cover the mock scenario we want | ||
| or it cannot handle some closure types, especially in function parameters. In such cases prefer to | ||
| create the mock manually and remove the protocol's `AutoMockable` comment. |
There was a problem hiding this comment.
⛏️ Grammar: Missing "to" before "create". Should be "prefer to create the mock manually".
| create the mock manually and remove the protocol's `AutoMockable` comment. | |
| create the mock manually and remove the protocol's `AutoMockable` comment. |
|
|
||
| #### Custom annotations | ||
|
|
||
| Sourcery allows us to annotate different parts of our code to guide code generation. Custom |
There was a problem hiding this comment.
💭 Clarity: This sentence structure could be improved for readability.
Suggested rewrite:
| Sourcery allows us to annotate different parts of our code to guide code generation. Custom | |
| Sourcery allows us to annotate different parts of our code to guide code generation. Custom annotations have been added in `AutoMockable.stencil` to handle special cases. |
| annotations have been added in `AutoMockable.stencil` to handle special cases. | ||
|
|
||
| - **useSelectorName**: Method annotation used to indicate that the generated mocked properties need | ||
| to use the selector name instead of the short method name. This is especially useful when using |
There was a problem hiding this comment.
⛏️ Grammar: "parameter names" (noun adjunct, preferred) is more natural than "parameter's names" (possessive).
| to use the selector name instead of the short method name. This is especially useful when using | |
| function overloading where we need the mocked names to also have the parameter names to |
🎟️ Tracking
PM-25300
📔 Objective
Adds docs on Sourcery usage on automatic mock generation on iOS architecture.
This has been added in bitwarden/ios#1724.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes