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

add Network-Conditions-Emulator #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add Network-Conditions-Emulator #6

wants to merge 1 commit into from

Conversation

marty90
Copy link

@marty90 marty90 commented May 18, 2018

No description provided.

@remingtonc remingtonc self-requested a review May 18, 2018 18:03
@remingtonc remingtonc assigned remingtonc and unassigned remingtonc May 18, 2018
Copy link
Contributor

@remingtonc remingtonc left a comment

Choose a reason for hiding this comment

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

Hi @marty90, this looks like a nice tc wrapper. I do have one pedantic concern which is the usage of the go function to arbitrarily eval input/replaces the go command in potentially sourcing scripts. Do you have any specific reason for using this go function instead of natively calling the commands? Otherwise, would it be possible to modify the code to natively call the commands?

Also, would you be okay with moving this to be a child bullet point of tc as it explicitly wraps tc functionality?

@marty90
Copy link
Author

marty90 commented May 18, 2018

Hi Remingtonc! No problem in putting it in a bullet of tc.. it is actually a wrapper that simplify the usage of tc... about the “go” macro: it was used just for debugging. uncommenting one line makes the bash print each command before executing. I agree that it is not super clean. I’m removing it in few days as I finish some tests :)

Thank you

@remingtonc
Copy link
Contributor

@marty90 Wonderful, ping me when it's ready to go and we will merge it in!

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

Successfully merging this pull request may close these issues.

2 participants