-
Notifications
You must be signed in to change notification settings - Fork 25
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 Validation Comments #3572
Add Validation Comments #3572
Conversation
d4c6a6b
to
b06f506
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this looks really good!! Don't be discouraged by the 10+ comments I added 😂 A lot of them are nit-picky about style or are slightly simpler ways to write things. Ultimately the code works, I just want us to get the code to a really good state!
I made some changes on the develop branch, and it looks like there are merge conflicts... Could you pull in the most recent changes from develop into your branch (if on your branch, running git pull origin develop
and then fixing any issues with it and testing again)?
Sorry for the styling issues, will make sure I review the guide before requesting review for new PRs. I commited changes for all the above. Thanks for the comments! |
I didn't check everything just now, but I left one comment, and I also noticed that you haven't pulled in changes from develop and resolved merge conflicts yet. From what else I saw though, it's looking real good! |
Thanks, just resolved merge conflicts. |
@cyrusnaficy you'll need to check to make sure that everything is working correctly after resolving merge conflicts! Just because you resolved everything that git noticed, doesn't mean that everything works properly! And taking a look at your merge, it looks like you left in the "<<<<<<< HEAD" lines and such, which are giving compilation errors! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you @cyrusnaficy !!
Resolves #3016
Added validation comments to the label map. If there are multiple comments, they will be seperated by a horizontal line. If there are no comments, it will display "None".
How response looks for labelID (ex.)
Before/After screenshots (if applicable)
Before
After
Testing instructions
Things to check before submitting the PR