-
Notifications
You must be signed in to change notification settings - Fork 135
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
Partial Fix for #83 (Excluding Gocyclo) #84
Conversation
Pull Request Test Coverage Report for Build 596
💛 - Coveralls |
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.
Thank you for the help with these issues. I have added a couple of suggestions below.
As for the Gocyclo issues - I have reviewed them and believe that we are fine ignoring those for now. Most of them are in the tests.
Added new linestyle LineStyleDouble
draw/line_style.go
Outdated
@@ -39,6 +39,19 @@ var lineStyleChars = map[LineStyle]map[linePart]rune{ | |||
vAndRight: '├', | |||
vAndH: '┼', | |||
}, | |||
LineStyleDouble: { |
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.
This looks great, thanks a lot!
Would you mind sending this in a separate pull request? A couple of reasons really:
- this being a new feature - I would prefer to merge it into the devel branch instead of the master and follow a release cycle.
- for the posterity - the pull request history or rollbacks become much cleaner when each pull request is focused on only one bug / feature / etc.
- we could iterate on this change a bit more without blocking your original PR regarding the lint issues. This new feature (new line style) could benefit from some test coverage. An advice I got a long time ago was to accompany each code change with at least minimal corresponding change in tests to "capture" the new feature in a working state.
I have no more comments on this PR without the new line style, so once we remove the line style from this scope - this is ready to go.
For (3) we could expand the test cases inside the border_test:
https://github.com/mum4k/termdash/blob/master/draw/border_test.go
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.
I've reverted the merge with the line-style.
I'll check out that test file 👍
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.
Thank you.
I fixed the errors except for gocyclo. Its an A+ grade now