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

add -l / --long flag #63

Merged
merged 7 commits into from Jul 10, 2017
Merged

add -l / --long flag #63

merged 7 commits into from Jul 10, 2017

Conversation

GladOSkar
Copy link
Contributor

As discussed in #40.

It became less elegant than i wanted it to be, so let me know if i can make it any better :)

Also while researching i found exa. They have a lot of similar and interesting features.

@athityakumar
Copy link
Owner

@GladOSkar - Can you also update the .travis.yml with the -l flag?

@GladOSkar
Copy link
Contributor Author

I just noticed i forgot to check rubocop and rspec. Give me some more time...

@GladOSkar
Copy link
Contributor Author

Why did this fail here? rubocop went fine on my machine...

@athityakumar
Copy link
Owner

@GladOSkar - Travis always tests on the current master. You might have tested rubocop on a previous master.

Anyways, seeing the Travis CI build report, I think ls -l (great work, BTW. Especially with the permissions coloring 😄 ) requires some improvements :

  • icon should be placed only before the filename, and not at the beginning of the line.
  • date and user could be yellow / white colored.

@GladOSkar
Copy link
Contributor Author

the date is currenntly green if it has been modified less than an hour ago, yellow if it has been modified less than 24 hours ago and white otherwise. if that is too complicated i can remove it.

the icon thing i can do of course

@athityakumar
Copy link
Owner

@GladOSkar - Ah, nice. Didn't see the code for the date coloring. This looks good as it is. 👍

Also, is there some issue with indentation when access permission on any one row is lengthier than the others?

stat = File.stat(content)
"#{mode_info(stat)} #{user_info(stat)} #{group_info(stat)} #{size_info(stat)} #{mtime_info(stat)}"
end

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe using a "\t" instead of space between each variable would do a better job of indenting? Like,

"#{mode_info(stat)}\t#{user_info(stat)}\t#{group_info(stat)}\t#{size_info(stat)}\t#{mtime_info(stat)}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it but it made way too big gaps. So i wrote some logic for that

Copy link
Contributor Author

@GladOSkar GladOSkar Jul 10, 2017

Choose a reason for hiding this comment

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

Currently, if the -l flag is passed, it calculates the longest user- and group name upon initialization and ljusts the names accordingly

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, looking at this screenshot, are the disturbances in lines 348 & 354 intended to be? (Like, 0;33;49m in rwx0;33;49mr-xr-x)

I think this is some unintended extension, and if taken care of, indentation should be proper by it's own.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, and i don't get how that can happen since the code just replaces ones and zeroes there... I'll have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you make that screenshot on your machine? If yes - what is your output of ls -l?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily, i do not see those Artifacts here: https://travis-ci.org/athityakumar/colorls/jobs/251976299

Copy link
Owner

Choose a reason for hiding this comment

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

Ah. I think I was looking at a previous build. Build 112 seems great! 👍

Copy link
Contributor Author

@GladOSkar GladOSkar Jul 10, 2017

Choose a reason for hiding this comment

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

OK so \e[0;33;49m is what has to be inserted i a terminal to produce yellow, and apparently travis swallowed 2-3 characters in each case, making the rest of the characters appear. i cannot image that happening in a real environemnt though

@GladOSkar
Copy link
Contributor Author

Can we just increase the class length limit or do i really have to make an extra class for that? I think it fits the way it is...

@athityakumar
Copy link
Owner

@GladOSkar - No need for a new class, but I think the class methods can be truncated to fit 200 lines. If not, we can temporarily increase it and fix it after the feature is added.

But of course, matching Rubocop's specs is favorable. 😄

end
user.to_s.length
end.max
end
Copy link
Owner

Choose a reason for hiding this comment

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

This begin-rescue block can be single-lined...

user = Etc.getpwuid(File.stat(c).uid).name rescue File.stat(c).uid

rescue ArgumentError
group = File.stat(c).gid
end
group.to_s.length
Copy link
Owner

Choose a reason for hiding this comment

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

... and so can this begin-rescue block.

rescue ArgumentError
group = stat.gid
end
group.to_s.ljust(@grouplength, ' ')
Copy link
Owner

Choose a reason for hiding this comment

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

... and this begin-rescue block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat.

@athityakumar
Copy link
Owner

@GladOSkar - Added some review comments to shorten the class length. 😄

@athityakumar
Copy link
Owner

@GladOSkar - Alright, let's change the .rubocop.yml to pass the checks & merge.

@GladOSkar
Copy link
Contributor Author

GladOSkar commented Jul 10, 2017

Now it doesn't like the rescues ^^

@athityakumar
Copy link
Owner

@GladOSkar - Hmm, revert the rescue changes and edit the .rubocop.yml with enough lines and perceived complexity to pass the Travis CI check. Let's have this merged and improve on it later.

@GladOSkar
Copy link
Contributor Author

I was just reading up on .try() , i think that may solve it. give me a sec

@GladOSkar
Copy link
Contributor Author

It didn't. But rubocop should be fine now.

@athityakumar athityakumar merged commit 39bfc7c into athityakumar:master Jul 10, 2017
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