-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix: correct regex match condition in SyntaxValidator #2384
base: master
Are you sure you want to change the base?
fix: correct regex match condition in SyntaxValidator #2384
Conversation
Signed-off-by: MAVRICK-1 <[email protected]>
2708e4d
to
beff3d6
Compare
@zFernand0 can you review my PR |
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.
- Could you add a quick changelog entry to the Imperative package?
packages/imperative/CHANGELOG.md
- See the contribution guidelines for an example
- Could you add a test to avoid this sort of issue in the future?
packages/imperative/src/cmd/src/syntax/__tests__/SyntaxValidator.unit.test.ts
@zFernand0 looking into it |
@zFernand0 I am solving the 1) in this PR , |
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2384 +/- ##
========================================
Coverage 91.28% 91.28%
========================================
Files 638 638
Lines 18207 18207
Branches 3822 3931 +109
========================================
Hits 16621 16621
Misses 1585 1585
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Hey @MAVRICK-1 , I think it'd be good to have the test added in the same PR 🙏 |
okay no issues🙂 on it
…On Fri, Dec 13, 2024 at 9:34 PM Fernando Rijo Cedeno < ***@***.***> wrote:
@zFernand0 I am solving the 1) in this PR , can solve no 2) in new PR?
Hey @MAVRICK-1 <https://github.com/MAVRICK-1> , I think it'd be good to
have the test added in the same PR 🙏
—
Reply to this email directly, view it on GitHub
<#2384 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BDBQOEKNJCWGG4KPBXO5VQL2FMAPZAVCNFSM6AAAAABTOHSBJ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBRG43DOMBVHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…positional argument validation test: add unit test for invalid positional argument regex validation Signed-off-by: MAVRICK-1 <[email protected]>
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.
Changes LGTM! 😋
left some comments to consider for improvement 🙏
Also, there is a small merge conflict on the imperative/changelog file 😋
packages/imperative/src/cmd/src/syntax/__tests__/SyntaxValidator.unit.test.ts
Outdated
Show resolved
Hide resolved
…regex-match-condition-in-SyntaxValidator
…sion test Signed-off-by: MAVRICK-1 <[email protected]>
@zFernand0 sry for the delay , it's fixed now |
Co-authored-by: Timothy Johnson <[email protected]> Signed-off-by: Rishi Mondal <[email protected]>
Co-authored-by: Timothy Johnson <[email protected]> Signed-off-by: Rishi Mondal <[email protected]>
@t1m0thyj Thanks it's done , now i understood the formate |
What It Does
fixed #2375
How to Test
Review Checklist
I certify that I have:
Additional Comments