Provides a smart way to determine the number 'top' lines #118

Merged
merged 5 commits into from Jun 3, 2016

Conversation

3 participants
@MinchinWeb
Contributor

MinchinWeb commented Mar 12, 2016

Looks at the prompt or PS1 environmental variables to determine the number of
newlines contained in them.

I've tested in on my end in Windows, and it works as expected. I don't have Linux or Mac box, so I haven't been able to test it there.

Because of the way it works (by counting newlines), it won't work exactly right if the prompt goes over the line end without a line break (i.e. for a really long path) or if there are conditional linebreaks in the PS1 variable.

Tests have been added and updated as well.

Fixes #102.

MinchinWeb added some commits Mar 12, 2016

Provides a smart way to determine the number of lines to print with t…
…he '-N' parameter.

Looks at the prompt or PS1 environmental variables to determine the number of
newlines contained in them.

Fixes 102.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 12, 2016

Current coverage is 87.48%

Merging #118 into master will decrease coverage by 9.98%

  1. 5 files (not in diff) in topydo/cli were created. more
  2. 1 files (not in diff) in topydo were created. more
  3. 2 files in topydo/lib were modified. more
    • Hits +7
  4. 1 files in topydo were modified. more
  5. 9 files (not in diff) in topydo/lib were modified. more
    • Misses -3
    • Partials -2
    • Hits -55
  6. 6 files (not in diff) in topydo/commands were modified. more
    • Partials +2
    • Hits +9
  7. 4 files (not in diff) in topydo/lib were deleted. more
@@             master    #118   diff @@
=======================================
  Files            50      52     +2   
  Lines          2562    2724   +162   
  Methods           0       0          
  Branches        406     415     +9   
=======================================
- Hits           2497    2383   -114   
- Misses           38     315   +277   
+ Partials         27      26     -1   

Powered by Codecov. Last updated by 14ab34d...b1e47ed

codecov-io commented Mar 12, 2016

Current coverage is 87.48%

Merging #118 into master will decrease coverage by 9.98%

  1. 5 files (not in diff) in topydo/cli were created. more
  2. 1 files (not in diff) in topydo were created. more
  3. 2 files in topydo/lib were modified. more
    • Hits +7
  4. 1 files in topydo were modified. more
  5. 9 files (not in diff) in topydo/lib were modified. more
    • Misses -3
    • Partials -2
    • Hits -55
  6. 6 files (not in diff) in topydo/commands were modified. more
    • Partials +2
    • Hits +9
  7. 4 files (not in diff) in topydo/lib were deleted. more
@@             master    #118   diff @@
=======================================
  Files            50      52     +2   
  Lines          2562    2724   +162   
  Methods           0       0          
  Branches        406     415     +9   
=======================================
- Hits           2497    2383   -114   
- Misses           38     315   +277   
+ Partials         27      26     -1   

Powered by Codecov. Last updated by 14ab34d...b1e47ed

No extra line on Linux
Windows typically prints a free line after program output; Linux doesn't.
@bram85

This comment has been minimized.

Show comment
Hide comment
@bram85

bram85 May 22, 2016

Owner

Change looks OK. Can you make sure to clean up the history such that Colors.py is not in the diff anymore? Then it looks OK to merge.

Owner

bram85 commented May 22, 2016

Change looks OK. Can you make sure to clean up the history such that Colors.py is not in the diff anymore? Then it looks OK to merge.

@MinchinWeb

This comment has been minimized.

Show comment
Hide comment
@MinchinWeb

MinchinWeb May 25, 2016

Contributor

@bram85 : Colours.py "fixed"

Contributor

MinchinWeb commented May 25, 2016

@bram85 : Colours.py "fixed"

@bram85

This comment has been minimized.

Show comment
Hide comment
@bram85

bram85 May 26, 2016

Owner

@MinchinWeb I had another look at your code, and I was wondering why you implemented _N_lines in the ListFormat.py module instead of ListCommand.py. The latter is the only client and I don't see much coherence with the formatting functionality (i.e. replacing placeholders). Did you do this for a particular reason? If not, I'd prefer to see this calculation in ListCommand.py.

Owner

bram85 commented May 26, 2016

@MinchinWeb I had another look at your code, and I was wondering why you implemented _N_lines in the ListFormat.py module instead of ListCommand.py. The latter is the only client and I don't see much coherence with the formatting functionality (i.e. replacing placeholders). Did you do this for a particular reason? If not, I'd prefer to see this calculation in ListCommand.py.

@MinchinWeb

This comment has been minimized.

Show comment
Hide comment
@MinchinWeb

MinchinWeb Jun 3, 2016

Contributor

@bram85 : the _N_lines function was placed were it was because that seemed to match (my limited understanding of) the organization used in the project. But no worries, I've moved it as you've suggested.

Contributor

MinchinWeb commented Jun 3, 2016

@bram85 : the _N_lines function was placed were it was because that seemed to match (my limited understanding of) the organization used in the project. But no worries, I've moved it as you've suggested.

@bram85 bram85 merged commit 1d19ce3 into bram85:master Jun 3, 2016

1 of 2 checks passed

codecov/project 97.32% (-0.14%) compared to 14ab34d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MinchinWeb MinchinWeb deleted the MinchinWeb:fix-102 branch Jun 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment