Skip to content

chore: expose methods for parsing api short name and version#1066

Closed
emmileaf wants to merge 2 commits intomainfrom
refactor-parse-defaulthost
Closed

chore: expose methods for parsing api short name and version#1066
emmileaf wants to merge 2 commits intomainfrom
refactor-parse-defaulthost

Conversation

@emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Oct 24, 2022

This PR exposes parseApiShortName (renamed from parseDefaultHost in #1040) and parseApiVersion (refactored from parsing proto package in prepareExecutableSamples) as public static methods in Composer.java. This change will enable Spring Codegen (when eventually split out from this repo) to reuse this parsing logic in descriptive comments and metadata.

An alternative refactoring to expose these two values as field accessors in the Service class can be found on the refactor-parse-defaulthost-2 branch. It moves the parsing logic to earlier in the process, since the source fields (defaultHost and protoPakkage) are both defined per-service. It’s a more convoluted change compared to this PR, but if the approach is preferred or adds any value from the sample generation perspective, happy to switch and open a PR from that branch instead.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@emmileaf emmileaf marked this pull request as ready for review October 24, 2022 13:58
@emmileaf emmileaf requested review from a team October 24, 2022 13:58
@blakeli0
Copy link
Contributor

blakeli0 commented Oct 28, 2022

Thanks Emily! Both approaches look pretty good! I would prefer the other one, even though it's more code but it opens up more opportunities in the future, if we need apiShortName or apiVersion to generate other stuff in the composers. I also think the other approach is cleaner instead of convoluted since it has a clear separation of concerns, the parsing logic is very specific to Service, it's not really Composer's concern.
If you really like this approach, I would at least extract the logic out to a util to make it cleaner. It would be weird if we call Composer.parseApiShortName() from another composer just for parsing.

@emmileaf
Copy link
Contributor Author

emmileaf commented Oct 28, 2022

Thanks @blakeli0! I was mainly concerned that the other approach is bringing too much change to the code for just one feature request from the spring side, but your rationale makes sense and it’s helpful to know this refactoring could benefit other development in the future. In this case, I’ll close this PR and open up another one from refactor-parse-defaulthost-2.

@emmileaf emmileaf closed this Oct 28, 2022
@emmileaf emmileaf mentioned this pull request Oct 31, 2022
Merged
@emmileaf emmileaf deleted the refactor-parse-defaulthost branch November 30, 2022 21:56
suztomo pushed a commit that referenced this pull request Mar 21, 2023
@renovate-bot
deps: update dependency com.google.code.gson:gson to v2.10.1 (#1066)
27f519d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhumin8 zhumin8 Awaiting requested review from zhumin8

@alicejli alicejli Awaiting requested review from alicejli

@blakeli0 blakeli0 Awaiting requested review from blakeli0

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@emmileaf @blakeli0