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

Modularize files and functions #92

Merged
merged 47 commits into from
Dec 16, 2012
Merged

Modularize files and functions #92

merged 47 commits into from
Dec 16, 2012

Conversation

jfelchner
Copy link
Contributor

This pull request cleans up the code quite a bit.

  • Readability improved
    • Whitespace is normalized. There were tabs mixed with spaces and trailing whitespace all over the place.
    • Blank lines added where it's important to break the (small) methods up into logical chunks
    • Indention level was set at 2 spaces
    • Tabs were removed and replaced with 2 spaces consistently throughout the lib and config directories
  • A lot of code has been rewritten such that
    • Almost all conditionals have been removed
    • Almost all calls to subshells (which are very slow) have been removed
    • A lot of duplicate code has been removed
    • Methods which were doing a bunch of things have been rewritten to do only one thing
    • There are only one set of methods for either side of the bar (with one exception) so there is no need to have status-left.sh and status-right.sh. Instead there is one powerbar.sh file which, when called with the proper side (eg powerbar.sh left) just does the right thing
    • Platform detection is now performed automatically using the $OSTYPE environment variable and functions provided to use in conditionals (eg shell_is_osx shell_is_linux)
    • Removed comments and instead made the code self-explanatory via named functions and variables
    • Made all variables and method naming consistent:
      • All configuration/global variables are in ALL_CAPS and start with TMUX_POWERBAR
      • All private methods begin with double underscores __ (this was already the case but I continued with it)
  • Files have been reorganized into more logical components (still some work to do here)
    • lib.sh especially was becoming a junk drawer with methods for everything from formatting to Tmux to muting the bar
  • Configuration of Powerbar has been rewritten to be able to be configured completely from environment variables and therefore the Powerbar files do not need to be touched
    • Moved from using associative arrays (which are very verbose in Bash) to indexed arrays which have their components in a set order. This takes the configuration which was 146 lines down to 26.

There's still some work I want to do here but I wanted to start getting feedback from everyone. Specifically:

  • If the bar is in the progress of being written and Tmux fires a refresh, the Powerbar process kicks off again, there needs to be a pid file that's checked for. If it exists, the subsequent process should quit silently.
  • The checks that were previously in the config file for 'if this platform then do this' or 'only enable this segment if on this platform' need to be removed. If there's something that needs to check the platform to determine which segment to load (eg the battery segment) that should be done inside a common 'battery' segment, which would load the proper sub-segment from there.
  • The proper segment files need to be sourced in and each segment should have a single method __<segment_name>_run which returns the text to display in the segment. This will eliminate all of the subshelling that's currently happening to execute all of the scripts and should give a noticeable speed improvement.

A few other things.

Comments are welcome.

Thanks!

…lly trample on a user's environment variables
…nd change the names in preparation for extraction
…ed out? Seems to me all items need an extra space on the right?
…s do the right thing

(Adds a couple too many spaces on the right but we'll fix that in a second)
@erikw
Copy link
Owner

erikw commented Dec 3, 2012

Wow that's a major PR, I'm glad your helping out. It will take some time to apply (will do as soon as I can); exams in one week. Have not looked at the actual changes but from the description it sounds good. About themes and ui-print functios, issue #81 should take care of that in a not to distance future (but might still pull in changes since it's better than the current state:))

@jfelchner
Copy link
Contributor Author

Yeah sorry for what ended up being a complete rewrite, I didn't mean for it to be this large of a PR. I had planned on making some simple changes, then those changes required other changes, etc, etc.

I think having a common codebase for Powerline segments would be a huge win. I had actually considered using Ruby or Python instead of Bash. It would make the code much cleaner, but I didn't want to add the extra dependency without asking first.

In the end, I think the code after the PR gets us most of the way there. Additionally, there are only two places in the code that even require Bash 4 now and we could probably use Bash 3 which would require no extra steps for 90% of the people out there.

I run ZSH 4 as my primary shell and I have Bash 4 installed as well but not everyone does.

@daviddavis
Copy link

This is awesome. 👍

We need this like yesterday.

@erikw
Copy link
Owner

erikw commented Dec 9, 2012

Indeed. I'll dedicate some time to this next weekend, after my finals :)

@greduan
Copy link

greduan commented Dec 13, 2012

I would love to have this. I'm actually waiting for this to be implemented in order to start using this. 😄

@erikw
Copy link
Owner

erikw commented Dec 15, 2012

Working on it right now, done later today. https://github.com/erikw/tmux-powerline/tree/refactor

@greduan
Copy link

greduan commented Dec 15, 2012

Excellent, looking forward to it. Thanks!

@erikw
Copy link
Owner

erikw commented Dec 15, 2012

Now this is going to be awesome. Getting late now, 6pm Friday gtg :) I'll pick up the work tomorrow. https://github.com/erikw/tmux-powerline/commits/refactor

@greduan
Copy link

greduan commented Dec 15, 2012

Excellent, thanks! Have a nice weekend. 😉

@jfelchner
Copy link
Contributor Author

@erikw I looked through all your commits. All of them look good to me with the exception of you converting all the spaces to tabs. What editor are you using that you need to use tabs instead of spaces? It causes serious problems when other people work on the code. A tab is not always the same for everyone. A space always is.

@jfelchner
Copy link
Contributor Author

Just as an example, look at how the diff looks on Github. Is this how it looks locally for you? I'd assume not. 363cc83

@greduan
Copy link

greduan commented Dec 15, 2012

@jfelchner I think that's why a tab would be better, first it's only one character, second, it can be any width the user wants, so it's more user friendly I would say. That's my opinion.

@erikw
Copy link
Owner

erikw commented Dec 15, 2012

While tempted to argue, all there is to say about tabs vs spaces has been said and I've picked my side :-)

@jfelchner
Copy link
Contributor Author

@greduan, you'd think so, it sounds logical, but it never works out that way. Turning on "soft tabs" in your editor gives you all the flexibility of tabs with none of the downsides. For example, look at that diff I linked to in my previous comment, tabs were used there, but the indention looks awful.

@jfelchner
Copy link
Contributor Author

@erikw I didn't even know it was something that was up for debate. :) Tabs give you zero benefit over using "soft tabs".

@erikagnvall
Copy link
Collaborator

@jfelchner Have you not been on the Internet? ^^ here's a pick of blogs about it:
http://lea.verou.me/2012/01/why-tabs-are-clearly-superior/
http://mystilleef.blogspot.se/2006/11/indentation-with-spaces-considered.html
http://www.rizzoweb.com/java/tabs-vs-spaces.html
http://blogs.msdn.com/b/cyrusn/archive/2004/09/14/229474.aspx
http://programmers.stackexchange.com/questions/57/tabs-versus-spaceswhat-is-the-proper-indentation-character-for-everything-in-e
http://www.codinghorror.com/blog/2009/04/death-to-the-space-infidels.html

Just Google "spaces vs tabs" and you'll find lots of weekend reading for both sides.

I agree with @erikw in the use of tabs but an argument about this never really gets anywhere. There are good arguments for both sides so it's more constructive to agree on one to use in the project. Since @erikw is the owner of the repo I'd say he gets the last say.

@greduan
Copy link

greduan commented Dec 15, 2012

I agree with @erikw and @meldanya, it's all about preference, and I think the owner of the repo gets the last say. 😉

@pablox-cl
Copy link

The tabs vs spaces discussion is fun, and let's face it: it's a holy war. Most arguments are frankly absurd (tabs saves disk space, OMG are we in 1990?), so I have to painfully agree with @meldanya and @greduan. It's up to the "team" to decide the coding style, since github is "person-centered" that is the repo owner. Maybe someday this project would belong to more than one person, then you (since I haven't contributed a single line of code) could discuss the coding style ❤️.


I have looked at the refactor branch, and I see that most Jeff contributions got in ONE commit, shouldn't be better if you rebase it to have a cleaner history? (lol, another holy war)

@erikw
Copy link
Owner

erikw commented Dec 15, 2012

Oh snap trapstorm, seems like I missed the -Xtheirs to the merge command. Sorry :-)

@daviddavis
Copy link

Personally, I like spaces over tabs but I agree that it should ultimately be up to the repo owner.

@daviddavis
Copy link

It looks like the README needs to be updated on the refactor branch to show how to use the new code...?

Is this something I can maybe help with? Thanks!

@erikw
Copy link
Owner

erikw commented Dec 15, 2012

Oh yeah the README needs a major update, forgot to put the refactor_TODO
file in the repo. If you happen to have a bash v. < 4 feel free to verify
that it works after the lib rewrite so we can get rid of that dependency
and all the OSX crap that comes with it.

On Dec 15, 2012 8:39 AM, "David Davis" [email protected] wrote:

It looks like the README needs to be updated on the refactor branch to
show how to use the new code...?

Is this something I can maybe help with? Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//pull/92#issuecomment-11406347.

@jfelchner
Copy link
Contributor Author

Holy crap. What did I start? :) My take is that if we only use tabs
that's fine. If we mix tabs and spaces like in the commit I referenced,
that's not. Consistency is more important than a dogmatic decision either
way.

It's @erikw's repo and I'll bow to the consensus. :) But my reasoning for
my concern was due to the... inconsistent indentation that was in the
codebase before I refactored it... which was due to spaces that were mixed
with tabs... which is a problem that has already been reintroduced

On Saturday, December 15, 2012, Pablo Olmos de Aguilera Corradini wrote:

The tabs vs spaces discussion is fun, and let's face it: it's a holy war.
Most arguments are frankly absurd (tabs saves disk space, OMG are we in
1990?), so I have to painfully agree with @Meldanyahttps://github.com/Meldanyaand
@greduan https://github.com/Greduan. It's up to the "team" to decide
the coding style, since github is "person-centered" that is the repo owner.
Maybe someday this project would belong to more than one person, then you
(since I haven't contributed a single line of code) could discuss the

coding style [image: ❤️].

I have looked at the refactor branch, and I see that most Jeff
contributions got in ONE commit, shouldn't be better if you rebase it to
have a cleaner history? (lol, another holy war)


Reply to this email directly or view it on GitHubhttps://github.com//pull/92#issuecomment-11405669.

@greduan
Copy link

greduan commented Dec 15, 2012

OH! I didn't understand it like that, sorry. :P

@erikw erikw merged commit 3ff0f06 into erikw:master Dec 16, 2012
@erikw
Copy link
Owner

erikw commented Dec 16, 2012

Woho finally done! Thanks a lot @jfelchner for starting this chain of changes. Just what this project needed :-) There's probably going to be some issues after this that has to bee fixed after such huge changes.

Some issues that I have not had time to fix yet tho: #101 #102

and I'm going on a two week computer-free holiday starting tomorrow but I may be able to communicate from my phone. I hope that I can hand over business to @meldanya for a while, if you have time buddy :)

@greduan
Copy link

greduan commented Dec 16, 2012

Excellent! Thanks for your hard work @erikw and @jfelchner!

@erikagnvall
Copy link
Collaborator

Good job! Much more pleasant to work with the code now :) I will try to steer the ship in your absence @erikw, but I can't guarantee that it won't hit an iceberg while I'm at the controls :) I do however have finals coming up this week so I won't be able to do much until this Friday.

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.

6 participants