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

Add a clarifying first paragraph to Future.md #623

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

dmk42
Copy link
Contributor

@dmk42 dmk42 commented Jan 11, 2024

This was requested at the 2024-01-11 meeting.

@dmk42 dmk42 requested a review from sthagen January 11, 2024 17:49
@dmk42 dmk42 self-assigned this Jan 11, 2024
@@ -1,5 +1,7 @@
# The Future of SARIF

The purpose of this document is to list potential new areas for standardization. These are not guarantees. They are areas we are strongly considering for a future edition of the SARIF standard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: "These areas are not guarantees."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of what is there. Are you saying replace the whole paragraph and say only this?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm saying to add "areas" after using the word "These" so that the reader knows what "These" refers to:
The purpose of this document is to list potential new areas for standardization. These areas are not guarantees. They are areas we are strongly considering for a future edition of the SARIF standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks. Yes, that's a good suggestion. I read right past the word "areas" and didn't see it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I'd be using the word "areas" three times. I'll think of something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it, the paragraph as a whole seems to work better the way it is, which avoids excessive repetition. If we had that sentence in isolation, then the change would work better.

@dmk42
Copy link
Contributor Author

dmk42 commented Jan 11, 2024

@Motional-Charles-Wilson , @davidmalcolm : I don't know why github won't let me add you as reviewers, but I'd appreciate it if you would take a look.

Copy link
Contributor

@sthagen sthagen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, David!

@dmk42 dmk42 requested a review from michaelcfanning January 11, 2024 18:25
@davidmalcolm
Copy link
Contributor

@Motional-Charles-Wilson , @davidmalcolm : I don't know why github won't let me add you as reviewers, but I'd appreciate it if you would take a look.

LGTM

@dmk42 dmk42 force-pushed the future_clarification branch from 9524caf to 3a67461 Compare January 24, 2024 23:25
@dmk42
Copy link
Contributor Author

dmk42 commented Jan 24, 2024

Thanks for taking a look. I will go ahead and merge now.

@dmk42 dmk42 merged commit 5f69bc4 into oasis-tcs:main Jan 24, 2024
@dmk42 dmk42 deleted the future_clarification branch January 24, 2024 23: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