Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

RangeMapped<T> functionality. #13

Merged
merged 10 commits into from
Jun 9, 2024
Merged

RangeMapped<T> functionality. #13

merged 10 commits into from
Jun 9, 2024

Conversation

John-Leitch
Copy link
Collaborator

Todo: say words about code

@John-Leitch John-Leitch requested a review from v0idzz June 5, 2024 04:45
@codecov Codecov
Copy link

codecov bot commented Jun 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.

Files Patch % Lines
...iler.Clr/Metadata/Tables/MetadataTableRowReader.cs 73.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

v0idzz
v0idzz reviewed Jun 5, 2024
View reviewed changes
Copy link
Owner

@v0idzz v0idzz left a comment

Choose a reason for hiding this comment

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

Mostly stylistic feedback. However, I believe the suggestion to make RangeMapped a struct is worth considering.

@John-Leitch John-Leitch requested a review from v0idzz June 6, 2024 04:26
v0idzz
v0idzz approved these changes Jun 6, 2024
View reviewed changes
Copy link
Owner

@v0idzz v0idzz left a comment

Choose a reason for hiding this comment

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

Looks good. I am happy with the class design here and I believe this is a step in the right direction.

@John-Leitch John-Leitch marked this pull request as ready for review June 8, 2024 23:08
@John-Leitch John-Leitch requested a review from v0idzz June 8, 2024 23:28
v0idzz
v0idzz approved these changes Jun 9, 2024
View reviewed changes
Copy link
Owner

@v0idzz v0idzz left a comment

Choose a reason for hiding this comment

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

Looks good!

@v0idzz v0idzz merged commit 66ced57 into v0idzz:main Jun 9, 2024
3 checks passed
@John-Leitch John-Leitch deleted the range-mapped branch June 10, 2024 02:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@v0idzz v0idzz v0idzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@John-Leitch @v0idzz