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.

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

Add validator for writing sam. #278

Merged
merged 7 commits into from
Nov 14, 2023
Merged

Add validator for writing sam. #278

merged 7 commits into from
Nov 14, 2023

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented Jun 15, 2023

I add sam1.0 validator for writing.
see also #271

@niyarin niyarin requested review from alumi and a team as code owners June 15, 2023 13:41
@niyarin niyarin requested review from ToshimitsuArai and removed request for a team June 15, 2023 13:41
@codecov
Copy link

codecov bot commented Jun 15, 2023 •
edited
Loading

Codecov Report

Merging #278 (9415ffe) into master (edecb9d) will increase coverage by 1.24%.
Report is 33 commits behind head on master.
The diff coverage is 69.31%.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   88.85%   90.10%   +1.24%     
==========================================
  Files          78       80       +2     
  Lines        6496     7981    +1485     
  Branches      456      687     +231     
==========================================
+ Hits         5772     7191    +1419     
- Misses        268      316      +48     
- Partials      456      474      +18
Files Coverage Δ
src/cljam/io/sam/util/validator.clj 69.31% <69.31%> (ø)

... and 44 files with indirect coverage changes

alumi
alumi requested changes Jun 25, 2023
View reviewed changes
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for adding this feature, @niyarin! 👍
As #277 is under review ahead of this, so please first apply the findings from #277 to this one and update this. Please also ensure that grammar, capitalization, punctuation, etc. are consistent.

@niyarin niyarin force-pushed the feature/check-writing-valid-sam branch from 2a5754f to 2343b68 Compare July 5, 2023 04:03
@niyarin niyarin force-pushed the feature/check-writing-valid-sam branch from f91ce3e to 8a9dfd3 Compare October 11, 2023 14:14
alumi
alumi reviewed Oct 12, 2023
View reviewed changes
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for updating! 👍 👍
I've added some comments mostly on wording.

src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/validator.clj Outdated Show resolved Hide resolved
@niyarin niyarin force-pushed the feature/check-writing-valid-sam branch from 77820a6 to f36a870 Compare November 1, 2023 02:06
@niyarin niyarin force-pushed the feature/check-writing-valid-sam branch from 3f8a705 to b1030cc Compare November 1, 2023 05:33
@niyarin niyarin requested a review from alumi November 2, 2023 02:15
@niyarin niyarin force-pushed the feature/check-writing-valid-sam branch from a19e9ba to 9415ffe Compare November 2, 2023 02:17
alumi
alumi approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the code, @niyarin!
LGTM 👍 👍

ToshimitsuArai
ToshimitsuArai approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@ToshimitsuArai ToshimitsuArai left a comment

Choose a reason for hiding this comment

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

Sorry for reviewing so late🙇‍♂️
LGTM👍

@ToshimitsuArai ToshimitsuArai merged commit b72d8a9 into master Nov 14, 2023
17 checks passed
@ToshimitsuArai ToshimitsuArai deleted the feature/check-writing-valid-sam branch November 14, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alumi alumi alumi approved these changes

@ToshimitsuArai ToshimitsuArai ToshimitsuArai approved these changes

Assignees

@alumi alumi

@ToshimitsuArai ToshimitsuArai

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@niyarin @alumi @ToshimitsuArai