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 the option to handle depth for the tree flag #253

Merged
merged 4 commits into from
Jan 11, 2019

Conversation

geektimus
Copy link
Contributor

@geektimus geektimus commented Jan 4, 2019

Introduction

Hi, I really like this gem, I was customising terminal and then, one day, I found myself running this in one of my personal projects with a lot of folders and I was surprised that nobody already fixed this so I wanted to improve in this part of the gem.

I'm not a full time ruby dev but I like to learn, Im open for suggestions and ideas 😃

Description

This pull request includes:

  • Changes required to handle depth while using the tree flag
  • Update the README to show the usage of the new option for tree
  • Update the README to remove the usage of "-t" as a short version of "--tree" since that definition is wrong. "--tree" is to show the directory tree and "-t" is to sort by modification time.

There's some work to be done to consider this as finished since I made one assumption that I want to clarify.

I need to check the other options/flags on the README to avoid cases as with "-t"

  • Relevant Issues : tree depth #168
  • Relevant PRs : None
  • Type of change :
    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation

feature-tree-traverse-depth

Note: Don't mind the missing icons on the gif. I'm learning how to use/customize that tool too 😅

@codecov-io
Copy link

codecov-io commented Jan 4, 2019

Codecov Report

Merging #253 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   84.37%   84.52%   +0.14%     
==========================================
  Files           7        7              
  Lines         416      420       +4     
==========================================
+ Hits          351      355       +4     
  Misses         65       65
Impacted Files Coverage Δ
lib/colorls/flags.rb 83.17% <100%> (+0.32%) ⬆️
lib/colorls/core.rb 88.78% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3897c6c...545630e. Read the comment docs.

@geektimus
Copy link
Contributor Author

Should I create a function for the new behaviour?
if the depth is not provided just use the old function and if the value is passed use the new one?

About the increment of the variable on the loop
tree_traverse("#{path}/#{content}", prespace + indent, depth, indent) unless depth > @tree[:depth] it was done there so I can use it for the function call and the unless condition.

I already re-write that part

@avdv
Copy link
Collaborator

avdv commented Jan 7, 2019

Should I create a function for the new behaviour?
if the depth is not provided just use the old function and if the value is passed use the new one?

No, let's not duplicate the code. Just use nil as default value for the depth option and handle it in the condition.

Also, could you add a test case in flags_spec.rb please?

And add an example call to the travis.yml file.

Copy link
Collaborator

@avdv avdv left a comment

Choose a reason for hiding this comment

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

Nice work, the PR is almost ready to merge. Could you fix the two minor issues, please?

spec/color_ls/flags_spec.rb Outdated Show resolved Hide resolved
lib/colorls/core.rb Outdated Show resolved Hide resolved
@avdv avdv merged commit c60a2eb into athityakumar:master Jan 11, 2019
@avdv
Copy link
Collaborator

avdv commented Jan 11, 2019

Thank you for your first contribution! 🎉

@avdv avdv mentioned this pull request Jan 11, 2019
5 tasks
@geektimus geektimus deleted the feature/tree_traverse_depth branch January 11, 2019 08:44
avdv pushed a commit to avdv/colorls that referenced this pull request Mar 11, 2019
* Add the option to handle depth for the tree flag

* Update README to describe the new option for the tree flag

* Add tests
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.

3 participants