-
Notifications
You must be signed in to change notification settings - Fork 26
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
Tuple missing types #65
Conversation
|
||
// Test with tuple arguments | ||
[DataRow("z.a.b((0,1))", "z", "a", "b((0,1))")] | ||
[DataRow("z.a.b((\"arg\",1))", "z", "a", "b((\"arg\",1))")] |
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.
@KonH Thanks for pointing this out.
Sorry for the delay in reviewing this. Here are some more complex test cases which currently fail. If you want to work on fixing this let me know. Otherwise, I can take more of a look in a few days.
[DataRow("z.a.b((0,1),(2,3))", "z", "a", "b((0,1),(2,3))")]
[DataRow("z.a.b((0,(0,1)),(0,1))", "z", "a", "b((0,(0,1)),(0,1))")]
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.
Thanks! I want, will be trying to fix it soon
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.
Sounds good, Thanks
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.
Add more test cases and push solution for them, now tests passed
I'm not sure about other corner cases, but that approach looks suitable
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.
But appveyor build failed for some reason (as you can see, dependencies aren't changed in that branch):
C:\Program Files\dotnet\sdk\3.1.401\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error NU5104: A stable release of a package should not have a prerelease dependency. Either modify the version spec of dependency "Stylecop.Analyzers [1.1.0-beta009, )" or update the version field in the nuspec. [C:\projects\nunit-testlogger\src\NUnit.Xml.TestLogger\NUnit.Xml.TestLogger.csproj]
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.
@KonH Thanks. I'm happy with the changes. The appveyor issue is from our tooling. I think you won't need to make changes for that.
@codito I think all 3 pulls are ready to be merged. #68 Adds the logo and would resolve all the appveyor issues. If we subsequently merge this and #67 then Master shouldn't have build issues.
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.
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.
Ok, so it was a simple fix when pulling it into the junit logger. spekt/junit.testlogger@94af6c5
Refers to comment here - spekt#65 (comment)
Instead of treat additional parentheses as an error in place, ensure that they are balanced (e.g., two closed and two open)
Refers to comment here - spekt#65 (comment)
…rt of tuple argument
a73abdb
to
141e96c
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.
Thanks @KonH for the fix!
Closes #64
The proposed solution to track extra parentheses:
Instead of treat additional parentheses as an error in place, ensure that they are balanced (e.g., two closed and two open)
New tests, covers tuple case, included