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

WIP: Simplify the library #68

Closed
wants to merge 14 commits into from

Conversation

MCWertGaming
Copy link
Collaborator

Hello, I'll try to simplify the code of this library in this pull-request. The main changes are turning most inline function into macros, move ansi codes into macros (so everyone can understand what the code actually does, because ansi is not human readable). changing some code logic to be smaller and cleaner. This will take me a bit, so I will post an update if the pull-request is ready to merge! But feel free to add notes, if you notice something i should change.

Best regards,
Damon

@certik
Copy link
Collaborator

certik commented Feb 8, 2021

I think changing functions to macros is not a good idea in C++, because the macros are not part of the Term namespace, and so they pollute user's namespace and also are less checked by the compiler for type errors. Rather, we should change all macros to functions.

See, e.g. these guidelines for more background and motivation:

@MCWertGaming
Copy link
Collaborator Author

I think that, if macros are bad due to changing the code to an alias, doing the same with a inline function is basically the same but requires to create objects (like the Term object which is just needed to call is_stdin_a_tty, which is just an inline function). I think that a macro does the same thing but without an object. Or we just create the inline function as anomymous function in the term namespace - would be the same but you don't need to create an object you use only one time. What do you think about that?

@certik
Copy link
Collaborator

certik commented Feb 8, 2021

I think we should use either functions in the Term namespace, such as Term::color(), or have them as static functions in the Term::Terminal class, so they can be called as Term::Terminal::is_stdin_a_tty() (no object/instance is created, this is a static function called directly from the class). The color function is useful even outside of the Terminal class, so that's why it was declared outside of it. The is_stdin_a_tty seemed to me it makes sense to be part of the Terminal class, but it could also be a separate function.

@MCWertGaming
Copy link
Collaborator Author

Yeah, i think you are right! I will move the functions to the Term namespace. I would generally minimize the functions of a class. What do you think? Rather all into the namespace or static functions?

I will revert the other definitions as well, sorry of that! I'm rather new to C++.

@certik
Copy link
Collaborator

certik commented Feb 9, 2021

No worries. Let's discuss on a case by case basis where a function should belong. It seems more separate functions in the Term namespace is the way to go.

@certik
Copy link
Collaborator

certik commented Feb 9, 2021

@MCWertGaming do you want to do a video call so that we can talk about the most useful way you can contribute? I have ideas how to improve the library and to add more examples to give people a better idea what it can do.

@MCWertGaming
Copy link
Collaborator Author

Sure, which plattform do you prefer? But I can only in the evening on workdays (CEST).

@certik
Copy link
Collaborator

certik commented Feb 9, 2021

I sent you a private email to arrange it.

@MCWertGaming
Copy link
Collaborator Author

All changes are split into seperate pull-requests.

@MCWertGaming MCWertGaming deleted the simplifying branch February 13, 2021 00:04
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