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

display which theme is the default one in basic output #2937

Merged
merged 2 commits into from
Apr 19, 2024
Merged

display which theme is the default one in basic output #2937

merged 2 commits into from
Apr 19, 2024

Conversation

sblondon
Copy link
Contributor

The previous PR #2838 shows which theme is the default one in colored output when --list-themes argument is used. This PR does the same with not colored output (as suggested by @Enselic ).

The output is:

bat_default_theme

I think about another commit to refactor the internal of list_themes() function (so it should not the squashed with the previous one).

Current code:

    if config.colored_output {
        for theme in assets.themes() {
            // display colored theme
            // display "Further themes can be installed..."
    } else {
        for theme in assets.themes() {
           // display basic theme
        }
    }

To:

    for theme in assets.themes() {
        if config.colored_output {
            // display colored theme
        } else {
           // display basic theme
       }
    }
    if config.colored_output {
            // display "Further themes can be installed..."
    } 

What do you think about it?

@sblondon
Copy link
Contributor Author

I added a new commit with the refactoring explained in the previous comment. So you can review it directly.

IMO, it's better with this refactoring.
However if you don't like it, I will drop the commit.

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks reasonable! I had one nitpick but we can fix that later, to avoid another round of back and forth.

Thanks!

CHANGELOG.md Show resolved Hide resolved
@Enselic Enselic merged commit bb4d1cb into sharkdp:master Apr 19, 2024
22 checks passed
@sblondon sblondon deleted the default-theme-without-colors-option branch April 21, 2024 08:56
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