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

Code improvements #73

Closed
wants to merge 28 commits into from
Closed

Conversation

Kreyren
Copy link
Contributor

@Kreyren Kreyren commented Jul 24, 2019

Fixing spellcheck and code quality

@Kreyren Kreyren force-pushed the code_improvements branch from fa54644 to 68f95be Compare July 24, 2019 01:33
@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 24, 2019

TODO: Error handling should be global

@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 25, 2019

Merged in https://github.com/RXT067/what-the-bash

Copy link
Member

@nntoan nntoan left a comment

Choose a reason for hiding this comment

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

This looks very good @Kreyren !! Appreciated it 💯

However, can you please also check my comments below?

Thanks,
Toan.

custom/aliases/example.aliases.sh Show resolved Hide resolved
oh-my-bash.bash Show resolved Hide resolved
@nntoan nntoan added community-feature Community feature merge request improvement and removed community-feature Community feature merge request labels Jul 26, 2019
@Kreyren Kreyren mentioned this pull request Jul 26, 2019
@Kreyren Kreyren self-assigned this Aug 15, 2019
@Kreyren Kreyren added the do-not-merge do not merge MRs label Aug 15, 2019
@Kreyren
Copy link
Contributor Author

Kreyren commented Aug 15, 2019

Do not merge untill issues are resolved.

@Kreyren
Copy link
Contributor Author

Kreyren commented Dec 28, 2021

_omb_log_die

I don't like the naming it's designed to be die as that is the well known function in other programming languages so it's not supposed to be specific to omb.

@Kreyren
Copy link
Contributor Author

Kreyren commented Dec 28, 2021

and convention for the other functions is e-name meaning enforced for phonetic communication which is a common convention for wrappers used in computer science to allow extensibility.

@akinomyoga
Copy link
Contributor

akinomyoga commented Dec 28, 2021

_omb_log_die

I don't like the naming it's designed to be die as that is the well known function in other programming languages so it's not supposed to be specific to omb.

Yeah, thank you for the comment! Actually, the naming convention is one of the topics that I wanted to discuss with other people. The problem is that oh-my-bash might be used with other Bash plugins outside oh-my-bash. Just as we are thinking that we want to define die, there are likely to be authors of other external plugins thinking that they want to define die. In addition, the users might also think they want to define the function die. The definition and the design of these implementations from different frameworks can be incompatible with each other, so there can arise real problems. Also, simpler names are more likely to conflict with real tools/commands.

In that case (of multiple definitions of die, etc.), how do you solve the problem? In well-structured programming languages, we typically use namespaces. If the user wants to import the names in the global namespace, something like alias die=_omb_log_die (akin to using omb::log::die; in C++, or from omb.log import die in Python) should be defined at the user's side but not at the library side. As you have encouraged us to use more organized programming languages such as Guile and Rust in #268, I think you can understand the situation. If oh-my-bash was the authority of the Bash/Shell scripting world and had the power to force all the other shell frameworks to be written on top of the oh-my-bash world, we might have argued the proper design of the shell language and utilities, but that's not the case, unfortunately.

My policy is that « we should not pollute the global interactive namespace with the functions for non-interactive uses, and only the functions that are supposed to be directly called from the command line should be placed in the global namespace ». This is the reason that the namespace _omb is started by the underscore _ implying it is not in the normal namespace. The functions die, ewarn, and einfo are clearly the components for a composite code and will not be called from the command line as a single command. Thus they fall into the non-global commands in my classification.

Let me hear what is your policy for avoiding conflicts! Or, do you think we should explicitly prohibit the oh-my-bash users from combining it with other external plugins such as fzf, bash-completion, etc.?

@akinomyoga
Copy link
Contributor

and convention for the other functions is e-name meaning enforced for phonetic communication which is a common convention for wrappers used in computer science to allow extensibility.

Thanks for the clarification! I once thought of sticking with the e prefix along with adding the namespaces, which resulted in something like _omb_log_e_warning, etc. I felt it too verbose so just dropped _e but actually there are no strong preferences for me in this aspect.

@akinomyoga akinomyoga dismissed their stale review December 28, 2021 16:35

Changes not requested anymore

@akinomyoga akinomyoga assigned akinomyoga and Kreyren and unassigned Kreyren Dec 28, 2021
@Kreyren
Copy link
Contributor Author

Kreyren commented Dec 29, 2021

I am aware of the presented issue thus why the provided definitions are prefixed with command -v function-name >/dev/null || function-name() { ... } so it's expected to work with multiple definitions that overwrite the provided on demand.

@akinomyoga
Copy link
Contributor

So that it can be replaced by the user-provided version of _omb_log_die, etc. if and only if the user intensionally replaces the logging function for OMB. Only in this way, we can ensure that the behavior of the replaced function is compatible with the original version of _omb_log_die.

@Kreyren
Copy link
Contributor Author

Kreyren commented Dec 29, 2021

i guess both options are valid due to how shell scripting is functionally limited

e.g. In rustlang i would just use use die::Die::die() as omb_log_die() which i see as an optimal solution here, but we can't really do outsourced through libraries on shell efficiently

Also as a lisp developer i find the _omb_log_die naming conversion super annoying as it's too long.

@akinomyoga
Copy link
Contributor

akinomyoga commented Dec 29, 2021

Thanks for the reply!

i guess both options are valid due to how shell scripting is functionally limited

Yes, I feel it really depends on the purpose. If we are developing an exclusive framework or a new shell language that can be used to develop a standalone shell application on top of the framework, it is good to define a set of basic utilities that shapes the style and designs of the scripting in the shell language.

However, oh-my-bash is not the framework to develop a standalone shell application, but primarily a shell configuration library for interactive sessions, where the main target of the shell namespace is the interactive commands that users input. The configuration such as oh-my-bash is inevitably just a subcomponent of the ecosystem but cannot be the main part that has control over everything. As I have already written, I am strongly opposed to forcibly defining die, etc. (which are not supposed to be used as the interactive commands) in the global namespace by oh-my-bash.

Of course, if you are the only user of oh-my-bash, you can control everything so can safely define die, ewarn, einfo, etc. Actually, it is completely valid that you have additional personal configurations of alias die=_omb_log_die, etc. in your local configuration. However, now we would like to discuss the design of the master version of oh-my-bash designed for arbitrary users/environments.

Also as a lisp developer i find the _omb_log_die naming conversion super annoying as it's too long.

Being a C++ programmer, I don't feel twelve characters are that long. It might be annoying for those who get used to a language with less flexibility (i.e. with many reserved names in the global namespace), but "super annoying" just hears like an exaggeration because recent strict languages tend to have longer fully-qualified (i.e. unaliased) names for outputting texts/logs, such as std::cout / System.out.println / System.Console.WriteLine / console.log, etc. I rather think it's just a matter of whether you get used to it, or it's an issue that can be compensated by tools such as autocompletion/predictions of the text editors.

I think we might think of renaming them to a bit shorter names _omb_die, etc., but I actually don't see any use of the function in the current codebase so reluctant to make it directly under the _omb namespace. In particular, die would be typically used to terminate the program on an error, but we wouldn't expect the interactive shell to be suddenly terminated by any error. At least, before moving to _omb_die, I would like to wait and see how it becomes to be used in the codebase for a wihle.

@Kreyren
Copy link
Contributor Author

Kreyren commented Dec 29, 2021

I also work with C++ and saying that lisp is less flexible is hilarious.. The point of flexibility in terms of naming identifiers is to make them quick to write while not creating conflicts with naming convention that clutter the code that infringes on readability, maintenance and file size where C/C++ are usually very guilty of doing that..

So i prefer identifiers to be at around 5 characters, but I am not too opposed to use C/C++-like naming here.

example of GNU Guile code for comparison:

#!/usr/local/bin/guile \
-l fact -e main -s
!#
;; define is used to define variables and functions
(define (choose n m)
  (/ (fact m) (* (fact (- m n)) (fact n))))

(define (main args)
  (let ((n (string->number (cadr args)))
        (m (string->number (caddr args))))
    (display (choose n m))
    (newline)))
$ ./choose 0 4
1
$ ./choose 1 4
4
$ ./choose 2 4
6
$ ./choose 3 4
4
$ ./choose 4 4
1
$ ./choose 50 100
100891344545564193334812497256

From https://www.gnu.org/software/guile/manual/html_node/Scripting-Examples.html

That said GNU Guile is extensible and functional language so it can't be compared to C/C++/Rust as they are different usecases e.g. I would use GNU Guile to build C++ if i was tasked to do something with C++, but i wouldn't use C++ to build GNU Guile.

@akinomyoga
Copy link
Contributor

I also work with C++ and saying that lisp is less flexible is hilarious..

Yeah, I think it was too hilarious. I haven't intended to say lisp is less flexible compared to C++ as a language, but I was specifically talking about the namespace or the identifiers that users can define in the global scope. For example, if the language or its standard library provides a global name die, it is considered a bad practice for a library to overwrite the function or to define another function of the same name in the local scope. It might be possible, but it will confuse the programmers. So die would be effectively reserved words, and users shouldn't define the function of the same name. That was what I was thinking in writing the previous reply.

The point of flexibility in terms of naming identifiers is to make them quick to write while not creating conflicts with naming convention that clutter the code that infringes on readability,

Are you talking about the identifiers in a local/limited scope? Isn't that the same with most languages that we can define short local identitiers? The example you have provided can be naturally translated to C++/Bash/etc. (though we might discuss the levels of optimizations in different languages). Anyway, I have been discussing the global names that are exposed to the users from the libraries (that are designed for the general users).

tools/git-prompt.bash Outdated Show resolved Hide resolved
tools/git-prompt.bash Outdated Show resolved Hide resolved
tools/git-prompt.bash Outdated Show resolved Hide resolved
tools/git-prompt.bash Outdated Show resolved Hide resolved
@Kreyren
Copy link
Contributor Author

Kreyren commented Jan 1, 2022

For example, if the language or its standard library provides a global name die, it is considered a bad practice for a library to overwrite the function or to define another function of the same name in the local scope. -- @akinomyoga (#73 (comment))

I do agree that:

die() { printf "FATAL: %s\\n" "$2"; exit 1 ;}

Is a bad practice thus why i am using:

command -v die >/dev/null || die() { printf "FATAL: %s\\n" "$2"; exit 1 ;}

Meaning:
a) if function die is NOT defined then define
b) if function die IS defined then use the already used

Which allows the user to set up their own handling of fatal errors in their ~/.bashrc for oh-my-bash to use e.g.

# ~/.bashrc
die() { printf "Something terrible happened: %s\\n" "$2"; exit 1 ;}

Are you talking about the identifiers in a local/limited scope? -- @akinomyoga (#73 (comment))

yes oh-my-bash should run in it's limited scope such as sub-shell (subshells bad!) to avoid exports to user shell that the user didn't requested.

The example you have provided can be naturally translated to C++/Bash/etc. -- @akinomyoga (#73 (comment))

yes it would be more appropriate to provide this in a different language to also enable translations as those are really painful to do in shell:

command -v die >/dev/null || die() {
  case "$LANG" in
    # Czech
    cs-*) printf "FATÁLNÍ" %s\\n" "$2" ;;
    # Default
    *) printf "FATAL: %s\\n" "$2"
  esac
  
  exit 1
}

In general shell should just be used as shell manager meaning to manage PS1, aliases and what gets sourced for PATH defining complicated functions and commands for production is malpractice.

The example you have provided can be naturally translated to C++/Bash/etc. -- -- @akinomyoga (#73 (comment))

Also don't think of shell as a C++ it has a very similar syntax, but it's more like very limited functional programming e.g. nixlang so it should be treated as such so defining very_long_identifier_that_does_things will eventually clutter the code and make it painful to work with, because you don't have the ability to source libraries in a standardized way for this kind of writting style to make sense.

Anyway, I have been discussing the global names that are exposed to the users from the libraries (that are designed for the general users). -- @akinomyoga (#73 (comment))

To clarify:

command -v die >/dev/null || die() { ... ;}

Is NOT being exported outside of omb, but it's affected by definition outside of omb.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 1, 2022

Is a bad practice thus why i am using:

command -v die >/dev/null || die() { printf "FATAL: %s\\n" "$2"; exit 1 ;}

Meaning: a) if function die is NOT defined then define b) if function die IS defined then use the already used

I don't understand. This doesn't solve anything. It seems like you have not yet understood what is the problem. As I have already written in the first reply to your question on _omb_log_die, the problem is

The definition and the design of these implementations from different frameworks can be incompatible with each other, so there can arise real problems.

How do you ensure that the existing die has the same assumption as OMB that the first argument is the exit status and the second argument is the message?

@akinomyoga
Copy link
Contributor

Are you talking about the identifiers in a local/limited scope? -- @akinomyoga (#73 (comment))

yes oh-my-bash should run in it's limited scope such as sub-shell (subshells bad!) to avoid exports to user shell that the user didn't requested.

But, the file lib/utils.sh is sourced in the main shell but not in sub-shells. If we want to define these functions only in the subshell as you say, then we need to write something like $(source lib/utils.sh; some_info_prompt). Is that what you suggest? Maybe that is another possible solution, but that is not the choice of the current codebase, so we need large refactoring of the entire codebase. Also, importantly, I don't think that approach is useful.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 1, 2022

The example you have provided can be naturally translated to C++/Bash/etc. -- @akinomyoga (#73 (comment))

yes it would be more appropriate to provide this in a different language to also enable translations as those are really painful to do in shell:

Hmm, maybe I misunderstood the reason why you have provided the example of (choose n m) written in Guile. I just thought that you have provided the example to show the flexibility of identifiers in Guile, so I just pointed that the example you have provided doesn't really show the flexibility of Guile compared to the other language because the example can be just rewritten in other languages. That was what I meant in the above sentence.

I was not talking about the translations of the general programs, for which I agree with you that the general program (which has a certain level of complexity) should not be written in shell scripts. I was talking about the specific program of (choose n m) that you have provided.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 1, 2022

The example you have provided can be naturally translated to C++/Bash/etc. -- -- @akinomyoga (#73 (comment))

Also don't think of shell as a C++ it has a very similar syntax, but it's more like very limited functional programming e.g. nixlang so it should be treated as such so defining very_long_identifier_that_does_things will eventually clutter the code and make it painful to work with, because you don't have the ability to source libraries in a standardized way for this kind of writting style to make sense.

Could you describe it in more detail? It appears to be the opposite for me. If there is a standardized way of importing libraries for the language, we expect from the language the standardized namespacing /scoping / module system, so we can define the identifiers in a namespace, etc. with short identifiers. However, if there is no standardized way of importing libraries, everything will be placed in the same namespace. In such a case, if every library defines functions of the same name but with different behavior, things will clutter. The only way of solving the situation is to protect ourselves by introducing effective namespaces by ourselves using the namespace prefixes to the identifiers.

Actually, you haven't answered the question in my first reply to your comment on _omb_log_die, which is closely related to this discussion.

Let me hear what is your policy for avoiding conflicts!

In case you didn't understand my question correctly, let me explain it again: If oh-my-bash would be allowed to define die, then the other libraries/frameworks should also be allowed to define die because oh-my-bash is not the standard of Bash. Also, the user might want to define die by themselves. However, the problem is that the design of these die can be different. For example, oh-my-bash expects die exit_status 'This is an error message', but the other libraries might expect die 'This is an error message' exit_status or just die This is an error message'. The user might want to define die for a completely different purpose.

@akinomyoga
Copy link
Contributor

Anyway, I have been discussing the global names that are exposed to the users from the libraries (that are designed for the general users). -- @akinomyoga (#73 (comment))

To clarify:

command -v die >/dev/null || die() { ... ;}

Is NOT being exported outside of omb, but it's affected by definition outside of omb.

Yes, that's the problem, or isn't that a problem that OMB is unexpectedly affected by the codes outside of OMB? I mean it is fine if the user intentionally changes the behavior of OMB by defining die, but the problem arises when the user or other libraries define die for their own purpose unrelated to OMB and the design of die is different.

It is more explicit, unambiguous, and readable that the user defines _omb_log_die (or possibly _omb_log) when the user intentionally replaces "die of OMB". Replacing die is less descriptive than _omb_log_die.

@akinomyoga
Copy link
Contributor

I dropped the commits related to these logging functions and merged the changes of this PR. I'm closing the PR now. We discuss the dropped commits for the logging functions at #290.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge do not merge MRs help-wanted Issues that require help from third party improvement P1 - Very Important Priority 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants