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.

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

adds the ability to set a custom writer (#5) #6

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

willmadison
Copy link
Contributor

@willmadison willmadison commented Oct 12, 2021

Closes #5.

@willmadison
Copy link
Contributor Author

If this looks good to you, please add the hacktoberfest-accepted label to this PR :)

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #6 (3163b8e) into main (b614ba8) will increase coverage by 8.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   17.89%   26.04%   +8.14%     
==========================================
  Files           4        4              
  Lines          95       96       +1     
==========================================
+ Hits           17       25       +8     
+ Misses         76       69       -7     
  Partials        2        2              
Impacted Files Coverage Δ
cursor_windows.go 27.90% <ø> (ø)
cursor.go 86.66% <100.00%> (+50.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b614ba8...3163b8e. Read the comment docs.

@MarvinJWendt
Copy link
Member

Hi @willmadison, thanks for the PR! Please also include your changes in the cursor_windows.go file :)

@willmadison
Copy link
Contributor Author

willmadison commented Oct 13, 2021

Hi @willmadison, thanks for the PR! Please also include your changes in the cursor_windows.go file :)

Happy to! I just wasn't sure that a generic writer would be congruent with the proprietary approach taken on the windows side given that io.Writers don't have a file descriptor which can be passed to syscall.Handle. What's the recommendation on how cursor_windows.go should allow for custom io.Writers in that context?

@MarvinJWendt
Copy link
Member

MarvinJWendt commented Oct 13, 2021

I am also not quite sure how we could do this on Windows. But we need the function SetTarget at least, so that the tests pass and the lib compiles cross-platform. Even if it doesn't do anything. BTW, could you also please add a short description to SetTarget, thanks. Maybe you could also add that setting a different target will not work for Windows 😉

@willmadison
Copy link
Contributor Author

I am also not quite sure how we could do this on Windows. But we need the function SetTarget at least, so that the tests pass and the lib compiles cross-platform. Even if it doesn't do anything. BTW, could you also please add a short description to SetTarget, thanks. Maybe you could also add that setting a different target will not work for Windows wink

Done! Also if you could please add the tag hacktoberfest-accepted to this PR that'd be awesome! Thanks!

@MarvinJWendt
Copy link
Member

Thanks @willmadison! I'd prefer not to add the hacktoberfest-accepted to the repo, as it will stick here forever then. You don't actually need the label anyway. Either the PR gets merged (which it will), or the label is added. Both scenarios will result in a finished Hacktoberfest PR 😄

@MarvinJWendt
Copy link
Member

PS: The tests fail under windows, maybe you could add a check inside the test, and if it's windows, just let it pass ^^

Copy link
Member

@MarvinJWendt MarvinJWendt left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 🚀

@MarvinJWendt MarvinJWendt merged commit 713fb85 into atomicgo:main Oct 13, 2021
@willmadison
Copy link
Contributor Author

willmadison commented Oct 13, 2021

@MarvinJWendt sorry to be a pest here, but it looks like things may have changed this year so if the PR (not the entire project) isn't opted in it won't get counted by the organizers. So if we could just tag the PR as 'hacktoberfest-accepted` that should suffice (without opting the entire project/repository in)

See more here:
Screenshot from 2021-10-13 16-43-59

@MarvinJWendt
Copy link
Member

MarvinJWendt commented Oct 14, 2021

Sorry! I forgot to opt this repo in. I thought we have the Hacktoberfest topic, as I added it to most of my recent projects, but as it seems not to this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to set custom io.writer
2 participants