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

fix: adjust validateOptions to handle preconfigured width #667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csabahenk
Copy link

fixes #666

main.go Outdated
@@ -156,6 +156,7 @@ func validateStyle(style string) error {

func validateOptions(cmd *cobra.Command) error {
// grab config values from Viper
width_in_config := viper.InConfig("width")

Choose a reason for hiding this comment

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

Suggested change
width_in_config := viper.InConfig("width")
widthInConfig := viper.InConfig("width")

main.go Outdated
@@ -156,6 +156,7 @@ func validateStyle(style string) error {

func validateOptions(cmd *cobra.Command) error {

Choose a reason for hiding this comment

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

Typo in commit and PR title

validateOptions not validaOptions

main.go Outdated
Comment on lines 179 to 198
// Detect terminal width
if !cmd.Flags().Changed("width") {
if isTerminal && width == 0 {
w, _, err := term.GetSize(int(os.Stdout.Fd()))
w := -1
if isTerminal {
var err error
w, _, err = term.GetSize(int(os.Stdout.Fd()))
if err == nil {
width = uint(w)
}

if width > 120 {
width = 120
if w > 120 {
w = 120
}
} else {
w = -1
}
}
if width == 0 {
width = 80
if w == -1 {
if !width_in_config {
width = 80
}
} else {
width = uint(w)
}
}
return nil
Copy link

@ccoVeille ccoVeille Dec 21, 2024

Choose a reason for hiding this comment

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

What about this:

func validateOptions(cmd *cobra.Command) error {
	width = viper.GetUint("width")
	mouse = viper.GetBool("mouse")
	pager = viper.GetBool("pager")
	showAllFiles = viper.GetBool("all")
	preserveNewLines = viper.GetBool("preserveNewLines")

	// validate the glamour style
	style = viper.GetString("style")
	if err := validateStyle(style); err != nil {
		return err
	}

	isTerminal := term.IsTerminal(int(os.Stdout.Fd()))
	// We want to use a special no-TTY style, when stdout is not a terminal
	// and there was no specific style passed by arg
	if !isTerminal && !cmd.Flags().Changed("style") {
		style = "notty"
	}

	// Detect terminal width
	if cmd.Flags().Changed("width") {
		return nil
	}
	if isTerminal {
		w, _, err := term.GetSize(int(os.Stdout.Fd()))
		if err == nil {
			width = uint(max(w, 120))
			return nil
		}
	}

	// grab config values from Viper
	widthInConfig := viper.InConfig("width")
	if !widthInConfig {
		width = 80
	}
	return nil
}

I wrote it on my phone in text mode, please test it and adapt

Copy link
Author

Choose a reason for hiding this comment

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

Fine and dandy, apart from having to have min(w, 120), not max :)

Choose a reason for hiding this comment

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

I'm glad you find it dandy 😁

Adding a Co-authored-by tag to your commit would be appreciated then

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>

Indeed for the min vs max

@csabahenk csabahenk changed the title fix: adjust validaOptions to handle preconfigured width fix: adjust validateOptions to handle preconfigured width Dec 21, 2024
fixes charmbracelet#666

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
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.

width calculation logic has the tacit assumption that no confiig file exists
2 participants