Skip to content

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

Merged
MarvinJWendt merged 3 commits into
atomicgo:mainfrom
willmadison:main
Oct 13, 2021
Merged

adds the ability to set a custom writer (#5)#6
MarvinJWendt merged 3 commits into
atomicgo:mainfrom
willmadison:main

Conversation

@willmadison

@willmadison willmadison commented Oct 12, 2021

Copy link
Copy Markdown
Contributor

Closes #5.

@willmadison

Copy link
Copy Markdown
Contributor Author

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

@codecov

codecov Bot commented Oct 13, 2021

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@willmadison

willmadison commented Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

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

MarvinJWendt commented Oct 13, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ^^

@MarvinJWendt MarvinJWendt left a comment

Copy link
Copy Markdown
Member

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

willmadison commented Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

@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

MarvinJWendt commented Oct 14, 2021

Copy link
Copy Markdown
Member

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