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

revise: wordsmith the explanation tags #349

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Pabsthegreat
Copy link
Contributor

@Pabsthegreat Pabsthegreat commented Mar 10, 2025

Requested change in issue #116 and wordsmithing explanation() texts

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added at least one relevant code reviewer to the PR.
  • Your code builds clean without any errors or warnings.
  • You have added an entry to the relevant CHANGELOG.md (see
    "keep a changelog" for more information).
  • Your commit messages follow the conventional commit style.
  • Your PR title follows the conventional commit style.

Requested change in issue stjude-rust-labs#116.
@Pabsthegreat
Copy link
Contributor Author

@a-frantz I'll try to flag some explanation tags that seem a little hard to understand in these comments. If there a better way to flag them please let me know. Also do I need to log an entry in CHANGELOG right now since this is quite a small edit?Thanks!

@a-frantz
Copy link
Member

If there a better way to flag them please let me know.

You can also create a thread in the #lib-wf-wdl GSoC slack channel! That should be a bit easier to discuss in.

Also do I need to log an entry in CHANGELOG right now since this is quite a small edit?

I'd wait for now, and add the CHANGELOG entry right before you mark this PR as ready for review.

@Pabsthegreat
Copy link
Contributor Author

Pabsthegreat commented Mar 10, 2025

I did go through some explanation texts, one thing I felt is including examples will be extremely helpful since they illustrate what the text is saying especially in long explanations like in expression_spacing.rs. I don't know if that will greatly increase the length of explanation but it sure would help in long explanations covering many rules. If readability is the aim and length is not an issue, then examples could get the point across without someone reading the text since code is sometimes easier to understand. @a-frantz

@Pabsthegreat
Copy link
Contributor Author

If there a better way to flag them please let me know.

You can also create a thread in the #lib-wf-wdl GSoC slack channel! That should be a bit easier to discuss in.

Also do I need to log an entry in CHANGELOG right now since this is quite a small edit?

I'd wait for now, and add the CHANGELOG entry right before you mark this PR as ready for review.

yes i will do that, thank you

@a-frantz
Copy link
Member

I did go through some explanation texts, one thing I felt is including examples will be extremely helpful since they illustrate what the text is saying especially in long explanations like in expression_spacing.rs. I don't know if that will greatly increase the length of explanation but it sure would help in long explanations covering many rules. @a-frantz

Examples would be great! I'm just not sure the best way to include them 🤔 I don't think they fit in the explanation() text itself, as that is more meant to be a justification for why the rule exists. So including examples there seems incongruous to me.

Perhaps we can add an examples() method to the Rule trait?

I'm open to ideas! But brainstorming is probably best done in the Slack to avoid too much noise here on github.

@Pabsthegreat
Copy link
Contributor Author

I did go through some explanation texts, one thing I felt is including examples will be extremely helpful since they illustrate what the text is saying especially in long explanations like in expression_spacing.rs. I don't know if that will greatly increase the length of explanation but it sure would help in long explanations covering many rules. @a-frantz

Examples would be great! I'm just not sure the best way to include them 🤔 I don't think they fit in the explanation() text itself, as that is more meant to be a justification for why the rule exists. So including examples there seems incongruous to me.

Perhaps we can add an examples() method to the Rule trait?

I'm open to ideas! But brainstorming is probably best done in the Slack to avoid too much noise here on github.

yes adding an examples() method might be great, yes ill move this conversation to Slack now. Thanks @a-frantz

@Pabsthegreat Pabsthegreat marked this pull request as ready for review March 12, 2025 07:18
@Pabsthegreat
Copy link
Contributor Author

@a-frantz @adthrasher could you please check what went wrong with the CI checks?

@Pabsthegreat Pabsthegreat requested a review from a-frantz March 12, 2025 07:24
@claymcleod claymcleod changed the title enchancement: wordsmithing explanation tags revise: wordsmith the explanation tags Mar 19, 2025
@claymcleod claymcleod added the S-needs-review State: awaiting initial or additional review label Mar 19, 2025
@claymcleod
Copy link
Member

Just in general, when referring to a literal code block (meta, parameter_meta, output), I'd wrap it in backticks to remain consistent with what we've done in the past.

@claymcleod claymcleod added S-awaiting-revisions State: awaiting revisions based on code review feedback and removed S-needs-review State: awaiting initial or additional review labels Mar 19, 2025
@claymcleod
Copy link
Member

Can you please fix the outstanding CI issue?

@claymcleod claymcleod added the S-awaiting-pass-CI State: awaiting the CI to pass label Mar 23, 2025
@claymcleod claymcleod force-pushed the main branch 3 times, most recently from 3dc8137 to f4c832c Compare March 26, 2025 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-pass-CI State: awaiting the CI to pass S-awaiting-revisions State: awaiting revisions based on code review feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants