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

Polished contributing document #10693

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Polished contributing document #10693

merged 2 commits into from
Feb 29, 2024

Conversation

steverao
Copy link
Contributor

@steverao steverao commented Feb 27, 2024

Polished contributing document

Keep first letter capitalized
@steverao steverao requested a review from a team February 27, 2024 16:13
@github-actions github-actions bot requested a review from theletterf February 27, 2024 16:13
@steverao steverao changed the title Update writing-instrumentation-module.md Keep first letter capitalized Feb 27, 2024
Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this is one sentence that has 2 embedded code snippets. In my opinion capitalizing it this way is not needed.

@steverao
Copy link
Contributor Author

steverao commented Feb 28, 2024

To me this is one sentence that has 2 embedded code snippets. In my opinion capitalizing it this way is not needed.

I think it's a little special here, If they are all in one sentence, without marks will make the sentence look a bit strange. I think the current way they are arranged, it is necessary to first letter capitalized, which looks more coordinate a bit. Sure, I can turn it off if you still don't think it's necessary?

@trask
Copy link
Member

trask commented Feb 28, 2024

@theletterf can you give us your expert opinion? Thanks!

@theletterf
Copy link
Member

So, when you're interspersing code snippets with sentences, it doesn't need a capital letter because it's all part of the same sentence. I think the snippets could use some restructuring though — here's my suggestion, which eliminates the issue and restores a semblance of capitalization. ;)

### Avoid using @Advice.Origin

You shouldn't use ByteBuddy's @Advice.Origin method, as it
inserts a call to `Class.getMethod(...)` in a transformed method.

Instead, get the declaring class and method name, as loading
constants from a constant pool is a much simpler operation.

For example:

```java
@Advice.Origin("#t") Class<?> declaringClass,
@Advice.Origin("#m") String methodName
```

@steverao
Copy link
Contributor Author

So, when you're interspersing code snippets with sentences, it doesn't need a capital letter because it's all part of the same sentence. I think the snippets could use some restructuring though — here's my suggestion, which eliminates the issue and restores a semblance of capitalization. ;)

### Avoid using @Advice.Origin

You shouldn't use ByteBuddy's @Advice.Origin method, as it
inserts a call to `Class.getMethod(...)` in a transformed method.

Instead, get the declaring class and method name, as loading
constants from a constant pool is a much simpler operation.

For example:

```java
@Advice.Origin("#t") Class<?> declaringClass,
@Advice.Origin("#m") String methodName

Thanks @theletterf! Except using @Advice.Origin Method method instead of @Advice.Origin method above. I think everything else is great! What do you think? @laurit @trask

@laurit
Copy link
Contributor

laurit commented Feb 28, 2024

@theletterf 's suggestion looks good.

@steverao steverao changed the title Keep first letter capitalized Polished contributing document Feb 29, 2024
@trask trask merged commit 822abc7 into open-telemetry:main Feb 29, 2024
49 checks passed
@steverao steverao deleted the patch-1 branch April 28, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants