-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fix typos and some redundancy #654
Conversation
Fixed a few typos and removed mentions of "from your bash compatible shell" (we use make so these commands are compatible with all clis)
Codecov Report
@@ Coverage Diff @@
## main #654 +/- ##
============================================
- Coverage 84.27% 84.27% -0.01%
+ Complexity 1152 1146 -6
============================================
Files 127 127
Lines 2786 2772 -14
============================================
- Hits 2348 2336 -12
+ Misses 438 436 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
README.md
Outdated
|
||
```bash | ||
make all | ||
``` | ||
Note: Copying `.env.dist` to `.env` fixes the warning ```The "PHP_USER" variable is not set. Defaulting to a blank string.``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is really a necessary setup step - can we move it somewhere prominent? I think the very first step after git clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, @brettmc, I agree. I wanted to do that but was slightly hesitant. I think it'll be placed better after the git clone
README.md
Outdated
@@ -142,15 +144,16 @@ In order to update all the vendored libraries in the `/vendor` directory. | |||
|
|||
Once you've made the update to the codebase that you'd like to submit, you may [create a pull request](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request) to the opentelemetry-php project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also reduce the visual noise of this? eg.:
"If you want to submit changes to the codebase, please create a pull request."
README.md
Outdated
|
||
You can simulate the important github actions locally before you submit your PR by running the following command: | ||
You can simulate the important GitHub actions locally before you submit your PR by running the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel this is wrong. This is not simulating GitHub actions, but is simply using the same commands as the CI.
README.md
Outdated
|
||
You can use the [examples/AlwaysOnNewrelicExample.php](/examples/AlwaysOnNewrelicExample.php) file to test out the reference implementation we have for Newrelic. This example perfoms a sample trace with spans and POSTs the result to a Newrelic endpoint. This requires a license key (free accounts available) set in the environment (NEW_RELIC_INSERT_KEY) to authenticate to the backend. | ||
You can use the [examples/AlwaysOnNewrelicExample.php](/examples/AlwaysOnNewrelicExample.php) file to test out the reference implementation we have for Newrelic. This example performs a sample trace with spans and POSTs the result to a Newrelic endpoint. This requires a license key (free accounts available) set in the environment (NEW_RELIC_INSERT_KEY) to authenticate to the backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove both NewRelic examples. This is not an advertisment platform for NewRelic, and the associated exporters may be moved to the contrib repo anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove both NewRelic examples. This is not an advertisment platform for NewRelic, and the associated exporters may be moved to the contrib repo anyway.
@tidal , Is there a specific reason why we are choosing to remove the NewRelic examples
@yuktea |
Fixed a few typos and removed mentions of "from your bash compatible shell" (we use make so these commands are compatible with all clis).
A part of [Issue #481]