Skip to content

Add bytes qualifier #28

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 9, 2023
Merged

Add bytes qualifier #28

merged 8 commits into from
Aug 9, 2023

Conversation

rdicosmo
Copy link
Contributor

Adding bytes qualifier, incorporating reviews from @zvr and @zacchiro on a first draft.

@rdicosmo rdicosmo mentioned this pull request Jul 28, 2023
Closed
@rdicosmo
Copy link
Contributor Author

@zvr, @zacchiro : this new PR incorporates the changes suggested in your reviews on #4 (that was wildly out of sync, hence this new PR)

@rdicosmo rdicosmo mentioned this pull request Jul 28, 2023
Closed
zacchiro
zacchiro requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@zacchiro zacchiro left a comment

Choose a reason for hiding this comment

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

Great, thanks for working on this. The bytes qualifier is much needed.

I've pointed out some changes that I believe are needed before landing it. Most of them are minor; and all them should be easy to address.

Roberto Di Cosmo added 4 commits August 2, 2023 17:55
Factor definition of range for bytes and lines qualifiers
3a2b5c0 
Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
Clarify that line numbers start from 1 and byte numbers start from 0
5adfc53 
Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
Recall explicitly the fact that bytes and lines qualifiers are mutual…
25c2cb0 
…ly exclusive.

Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
Prescribe behaviour when bytes and lines qualifiers are both present.
f6c0eaf 
Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
@rdicosmo rdicosmo requested a review from zacchiro August 2, 2023 16:14
zvr
zvr requested changes Aug 2, 2023
View reviewed changes
zacchiro
zacchiro previously requested changes Aug 3, 2023
View reviewed changes
Clarify that range includes borders
b82a54b 
Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
@rdicosmo rdicosmo requested review from zvr and zacchiro August 3, 2023 12:07
Improve workding of section on bytes/lines mutual exclusion.
1649c29 
Signed-off-by: Roberto Di Cosmo <roberto@dicosmo.org>
@rdicosmo rdicosmo self-assigned this Aug 5, 2023
@rdicosmo rdicosmo dismissed zacchiro’s stale review August 5, 2023 18:17

All feedback has been incorporate.

@zvr zvr merged commit 04ec38f into swhid:main Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvr zvr zvr approved these changes

@zacchiro zacchiro zacchiro approved these changes

Assignees

@rdicosmo rdicosmo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rdicosmo @zvr @zacchiro