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

Wind Direction Letters #58

Closed
wants to merge 2 commits into from
Closed

Wind Direction Letters #58

wants to merge 2 commits into from

Conversation

jwest23
Copy link
Contributor

@jwest23 jwest23 commented Sep 16, 2015

I added these two changes to my fork of wego so I could get letters instead of the arrow icons for wind direction. Please add these changes if they're a good fit.

Thanks for wego!

@schachmat
Copy link
Owner

Heyho @jwest23, I have a few questions regarding your PR:

Why do you think the pad function is less fragile? Imho it is even a regression, since it is not able to trim the provided string, if it is too long. Also it still leaves the problem, that you have to count the escape codes for coloring. If both of these problems can be fixed with a new pad() function, I would be happy to apply it.

In which case would you want to fall back to wind direction letters instead of the arrows?

@jwest23
Copy link
Contributor Author

jwest23 commented Sep 18, 2015

Hello!

"less fragile" is an overstatement since the pad function introduces other problems, for sure. The pad function only allows for variability in the length of the display string that I couldn't manage with just a slice. I'll take a look at incorporating both halves of the functionality.

The only reason I wanted this is because one of my terminal setups wasn't displaying the diagonal arrows properly, and instead of fixing the font I figured I'd take the opportunity to learn a little go. It's only a marginally useful feature, at best.

Thanks again!

@schachmat
Copy link
Owner

Ah, now I see, why you needed the pad function for your patch ('NE' is a character more than the arrow). With a proper pad function as described above it can be used for the other formatBLA() functions as well. On truncation it should replace the last character with the '…' Ellipsis character.

For the letters: Please fix your font. The requirements from the README state an 'utf-8 terminal with 256 colors'. Your workaround is well implemented, but not needed and I want to encourage people to use utf-8 if they don't already.

@jwest23
Copy link
Contributor Author

jwest23 commented Sep 29, 2015

"For the letters: Please fix your font."

I'm going to take this to mean that, in the long run, the main motivation behind the pull request isn't desired. Therefore, I'm closing this pull request.

@jwest23 jwest23 closed this Sep 29, 2015
@jwest23 jwest23 deleted the enhancement/WindDirLetters branch September 29, 2015 13:39
@schachmat
Copy link
Owner

I managed to change your pad() function to support all the needed features and merged that at least. I left you as the author of the commit, since it was your idea. Thanks for the contribution @jwest23.

@jwest23
Copy link
Contributor Author

jwest23 commented Sep 30, 2015

Thanks for giving me credit when you did the work. Glad I could make even
a small contribution.

And thanks again for wego!
On Sep 30, 2015 7:02 PM, "schachmat" notifications@github.com wrote:

I managed to change your pad() function to support all the needed
features and merged that at least. I left you as the author of the commit,
since it was your idea. Thanks for the contribution @jwest23
https://github.com/jwest23.


Reply to this email directly or view it on GitHub
#58 (comment).

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