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

Implemented hardmin feature #47

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Implemented hardmin feature #47

merged 2 commits into from
Apr 26, 2021

Conversation

hrshbh
Copy link
Contributor

@hrshbh hrshbh commented Apr 23, 2020

Implemented the hardmin feature, as requested in issue #42 and issue #22 , along with other minor improvements.
softmin not implemented.

@tenox7
Copy link
Owner

tenox7 commented Apr 26, 2020

Very nice! However you have also made some changes that do not concern hardmin feature. This makes it hard to read. Please split this PR in to smaller ones so that they can be addressed in a more granular meaner. Thank you!

@hrshbh
Copy link
Contributor Author

hrshbh commented Apr 26, 2020

Done. Now all changes strictly concern with hardmin.

@tenox7
Copy link
Owner

tenox7 commented Apr 26, 2020

I would also like the other changes, just in a separate PR. Thank you!

Copy link
Owner

@tenox7 tenox7 left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this. Works mostly awesome except the weird softmin issue and the usage/manual could use some improvement.

README.md Outdated Show resolved Hide resolved
ttyplot.1 Outdated Show resolved Hide resolved
ttyplot.c Show resolved Hide resolved
@hrshbh
Copy link
Contributor Author

hrshbh commented Apr 26, 2020

I agree, I was sloppy about the usage() and the manpage. I've tried to make them better, but if still changes are required in them, then I think you can amend it here before merging.

Regarding the softmin matter, I have added checks after parsing of args.

And as to the error response of value being less than hardmin, I was thinking of making as little changes as possible to the code, as it was beautiful already. Now, I've added a new error symbol, and included option to modify it.

@hrshbh hrshbh closed this Jun 28, 2020
@hrshbh hrshbh deleted the hardmin branch June 28, 2020 17:47
@tenox7
Copy link
Owner

tenox7 commented Mar 17, 2021

Hi, why did you close this? I would really like to get min/hardmin in to ttyplot.

@hrshbh
Copy link
Contributor Author

hrshbh commented Mar 20, 2021

Hi, the PR was in the open state for quite some time, without any response. So, I decided to close it.
Should I reopen this?
It's been almost a year, probably there will be some merge conflicts that will need to be resolved first.

@tenox7
Copy link
Owner

tenox7 commented Apr 21, 2021

I really want to have it so I guess reopen it?

@hrshbh hrshbh restored the hardmin branch April 22, 2021 11:12
@hrshbh
Copy link
Contributor Author

hrshbh commented Apr 22, 2021

Thanks for the interest.
Will soon work to resolve the merge conflicts, if any.

@hrshbh hrshbh reopened this Apr 22, 2021
@tenox7
Copy link
Owner

tenox7 commented Apr 22, 2021

thanks! let me try it!

@tenox7 tenox7 merged commit 9ce0ea3 into tenox7:master Apr 26, 2021
@tenox7
Copy link
Owner

tenox7 commented Apr 26, 2021

Merged thank you very much!

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.

None yet

2 participants