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

Got rid of IsTerminal call to reduce external dependencies #930

Closed
wants to merge 5 commits into from
Closed

Got rid of IsTerminal call to reduce external dependencies #930

wants to merge 5 commits into from

Conversation

tandr
Copy link
Contributor

@tandr tandr commented Mar 26, 2019

Addresses #928

@@ -1,9 +1,9 @@
// +build !appengine,!js,!windows,aix
// +build aix
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this, at least, be moved to a subpackage with a doc.go explaining where it comes from and why it was done this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I follow, sorry. What should I move into doc.go ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Move all the files you just added to a terminal package (sub folder) and create a doc.go in there to add some comments.

Copy link
Contributor Author

@tandr tandr Mar 26, 2019

Choose a reason for hiding this comment

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

Right now all the functions exposed by this terminal_* group (I added only 2 files, but could've done with 1 in retrospect) are not exported (checkIfTerminal()). If we want to keep it that way, they will need to go into "internal/terminal" instead, so they will become un-importable from any other projects. This change scope will grow. We can do it in a separate MR or do it here. How do you want to proceed ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can create a IsTerminal function in the package, which would be public, just like it is in the ssh/terminal package. That way, no need to change any other file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to change other files to adjust imports. I moved all of them under import/terminal, please take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add an additional review since there is more than one way to solve it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #932 that be closer to what you have asked

@tandr
Copy link
Contributor Author

tandr commented Mar 27, 2019

#932 solves it in a maybe a less structured (some of the responsibilities of detecting platform moved, not all of them), but definitely less intrusive way

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.

2 participants