Closed astrohart closed 3 years ago
Hi @astrohart ,
Sorry for the delay. Just got out of work. Thank you for reaching out and raising the issue.
Yes, let's change the exception message to be more consistent: https://github.com/bchavez/Coinbase/blob/67dd4582ab5eaa042232c5d32c332cee613da763/Source/Coinbase/Config.cs#L66
Feel free to send a PR; or I can work on it this weekend. I apologize in advance if my responses are slow, but I'll catch up on the weekend.
Thanks, Brian
Hi @bchavez
I'd make the change and pull request. However, as a best-practice, I like to do a build prior to pushing to the remote. The reason being, is that in general, one wants to only push building code to a remote repository, be it a fork or otherwise. The only issue is, the project is configured to automatically push to your master
branch on build.
Do you know of any way I can alter the forked version so as (a) not to actually put a change in the PR that you don't want, i.e., not changing the config of that, and (b) to avoid pushing to your master
branch by mistake.
-Brian
Hi @astrohart,
What part of this repository do you see that something pushes to master on build?
Are you talking about the appveyor.yml
file? If so, that file has no effect on your local machine. The appveyor.yml
is only relevant when code is pushed to this repo when being built under the CI/CD server on AppVeyor.
You don't have to worry about pushing or over-writing my changes on master
here on GitHub. By default, permissions on this repo will deny any attempts to push or write to this repository to anyone. The only person that can write directly to master
is me. You can submit a PR, but that won't have any "effects" here on master
until I merge the PR.
I see your point. Yes, I am somewhat vaguely familiar with AppVeyor (and when I say "vaguely," I mean like at the 0.01% level).
Remember back when I cloned the repo way back in the day and just started refactoring it to high heaven and that created consternation for you? First of all, in my GitHub dumbness, I had cloned it off your master directly. I've since advanced considerably in my understanding of how to contribute to people's projects by then..
See the image above. My concern stems from the package shown circled. Does that NuGet package not provide the functionality to automatically commit changes and push to your repo (not my fork) when I build my cloned fork's files? I am worried that, even if I, say, click Build Solution
i Visual Studio, it will still do a push to your master
branch due to the fact that there is also a Builder.fsproj
file in the solution and its configuration points at your bchavez
GitHub.
So, here's what I suggest. I've gone ahead and forked your repo to my astrohart
GitHub account. I've now cloned the fork to my local machine. Then, I suggest I make the change as I have proposed at the top of this issue thread and just do a build. Let's test whose repo that it pushes to (if any) as a result.
Please don't get mad at me, I am just testing to make sure I am not accidentally stepping on your toes.
OK, so I made the change and now have done the build. I have just now looked at your master
version of Config.cs
and identified that there have been no changes at all pushed to your repo by my mere building.
Therefore, let me just commit my change and push it to the fork, and then I will submit a PR.
See PR #78
Re: Microsoft.SourceLink.GitHub, is used for CI builds only. SourceLink should have no impact on local developer builds.
When AppVeyor is producing a build for NuGet, SourceLink is included into the build process. SourceLink helps generate "github-ified" debug symbols that link to the exact commit that is currently being built from GitHub. For example, if you have a debugger running and you "step into" the Coinbase library; the Visual Studio debugger + the SourceLink'ed PDBs should "download" the source-code from my GitHub repository and then step-into whatever function your debugging. You can read more about SourceLink here: https://devblogs.microsoft.com/dotnet/producing-packages-with-source-link/
The process you describe is conceptually how PRs on GitHub should be done:
bchavez/Coinbase
to username/Coinbase
.username/Coinbase
; change whatever you want. There are no limitations while on your fork.username/Coinbase
to bchavez/Coinbase
; this is the step that requires approval and review.GitHub, by default, will not allow anyone to push or write to bchavez/Coinbase
; in whatever fashion. You can perform a small test; it won't work. You should get a 403 forbidden from your GitHub client if you try to push anything to bchavez/Coinbase
. So, there's nothing to worry about.
In general, make sure your PRs follow these principles:
And that's about it.
In the
Coinbase.ApiKeyConfig.EnsureValid
method, there is the line:Respectfully suggest altering the error message code to read:
The API Secret must be specified.
, to be consistent with the check.