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

Diversifying the reflection API usage, for DeploymentReader test #1134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

advancedwebdeveloper
Copy link

Consult the following discussions, for farther details:
https://groups.google.com/g/golang-nuts/c/S2gBW3BV4QU/m/I4gWtrPxBwAJ
#1130

Compared to a summary, known threw a patch https://gist.github.com/advancedwebdeveloper/9ee1d333d0101b06823735cf871055b1 - the code above was not formatted automatically, via https://pkg.go.dev/golang.org/x/tools/cmd/goimports

@mrutkows
Copy link
Contributor

mrutkows commented Mar 24, 2021

Although I have read the attached discussion thread (which itself lacked context) I am not inclined to begin using a 3rd party alternative unless there is a compelling reason to especially if there are no apparent functional issues with the code as it exists today. In addition, it seems that the actual owner of the "reflect" lib in golang itself is trying to understand some minor optimization and resultant performance issues and fix them in the base golang; if so, then I am inclined to wait for those changes to be picked up in an actual golang release/update.

Also, we should not be using Travis PR builds simply to use as an alternative local test bed; the ASF pays for Travis.com usage and we should not be abusing the privilege. Unit tests for your purposes (i.e., testing a new lib.) are sufficient until you are ready to submit a final PR with the complete solution along with additional unit tests. If indeed, you are fixing a problem, we should have an issue for it that provides continuity of discussion on the topic (here in this project) and it should be linked to the eventual PR. I still do not have a compelling answer as to what problem you are trying to solve other than inferring that you believe that our client tools must support some divergent ARM64 architecture (guessing M1) more quickly than the base golang compiler does. IMO, we would need the compelling argument to go down this path for not only this tool, but also for all the client tooling within the entire project that uses golang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Do not merge this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants