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

API change - progress bar can be constructed with proper settings. #21

Merged
merged 9 commits into from Feb 11, 2020

Conversation

dawidpilarski
Copy link
Contributor

Hi there!

I would like to ask whether you would be open to the following changes.

The rationale behind them is to make it possible, to create ProgressBar object with options given as constructor parameters, as with current approach it would extremaly difficult to do so.

@dawidpilarski
Copy link
Contributor Author

I fixed the Codacy complaining about non-explicit ctor, even though, I am not convinced, whether the constructor really should be explicit in this case.

I cannot, however, fix the complaints about always_true::value is not used as well as are_setting::value, because Codacy is simply wrong in this case.

@p-ranav
Copy link
Owner

p-ranav commented Feb 3, 2020

I'm open to this change. Can you update the README too? I can't merge until the README is consistent with the changes.

While we're at it, we should probably change enums to all lower case per https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-enum

@dawidpilarski
Copy link
Contributor Author

I'm open to this change. Can you update the README too? I can't merge until the README is consistent with the changes.

Sure I will. I should also most probably change other types than ProgressBar to use new kind of settings before merging this PR, right?

@p-ranav
Copy link
Owner

p-ranav commented Feb 4, 2020

Yeah

@dawidpilarski
Copy link
Contributor Author

@p-ranav I think all the changes are in place. Please review them carefully, especially settings that we expose. If something is still wrong, please let me know :)

include/indicators/progress_bar.hpp Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
demo/demo.cpp Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@p-ranav
Copy link
Owner

p-ranav commented Feb 11, 2020

@p-ranav I think all the changes are in place. Please review them carefully, especially settings that we expose. If something is still wrong, please let me know :)

Thanks for the great work! I have a lot I can learn from your implementation. I've added comments referencing some minor things.

@dawidpilarski
Copy link
Contributor Author

Glad I could help!

@p-ranav p-ranav merged commit 764d796 into p-ranav:master Feb 11, 2020
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