Add percent column #258

BrainMaestro · 2018-09-05T12:26:43Z

Fixes #230

BrainMaestro · 2018-09-05T12:28:17Z

src/language/mod.rs

+
+    fn add(self, rhs: &Self) -> Self {
+        let mut stats = self.stats;
+        stats.extend(rhs.stats.clone());


The clone here is unfortunate, the only way I see around it is for stats to be Vec<Rc<Stat>>, do you have a better solution?

BrainMaestro · 2018-09-05T12:29:09Z

I decided not to add a test for this because it was basically pointless since percentage is 100% for the individual files.

BrainMaestro · 2018-09-05T12:29:31Z

src/language/mod.rs

@@ -29,6 +30,8 @@ pub struct Language {
    pub lines: usize,
    /// A collection of statistics based on the files provide from `files`
    pub stats: Vec<Stats>,
+    /// Total percentage of the language in the project
+    pub percent: usize,


would you prefer a float?

XAMPPRocky · 2018-09-05T14:16:09Z

Thank you for your PR! I'm sorry I should have given more guidance and said what I think the feature should be. As I didn't actually want to add a column. The way I saw this working was behind a flag. I also saw instead of and extra column I saw the output looking like below. Where it showed the absolute and relative values of each compared to the total. For the —files output they would be compared to the language.

--------------------------------------------------------
 Language                                        Files
--------------------------------------------------------
 Batch                                         1|0.29%
 C                                             1|0.29%
 Makefile                                      1|0.29%
 Markdown                                     19|5.5%
 Python                                       75|22%
 ReStructuredText                              7|2%
 Rust                                        210|61%
 Shell                                         7|2%
 TOML                                         17|5%
 Vim Script                                    2|0.58%
-------------------------------------------------------
 Total                                          340
-------------------------------------------------------

BrainMaestro · 2018-09-05T14:32:45Z

That is indeed much better. I wasn't comfortable with the way it was implemented anyway. I'll fix

XAMPPRocky · 2018-09-05T14:34:11Z

@BrainMaestro Thank you for working on this, and if you have any questions don't hesitate to ask.

BrainMaestro · 2018-09-06T14:59:04Z

See what it looks like

 Language              Files        Lines         Code     Comments       Blanks
---------------------------------------------------------------------------------
 JSON               1|23.26%         1622         1622            0            0
 Markdown           5|26.40%         1841         1841            0            0
 Rust              13|29.43%         2052         1557          207          288
 Shell              4|1.84%           128           92            8           28
 SVG                8|16.72%         1166         1126            0           40
 TOML               1|1.20%            84           69            0           15
 YAML               1|1.15%            80           60            8           12
---------------------------------------------------------------------------------
 Total                    33         6973         6367          223          383
---------------------------------------------------------------------------------

Should the total files be aligned with the files not the percentages?

XAMPPRocky · 2018-09-06T16:27:54Z

@BrainMaestro I think aligned with the number but I'll have to see to confirm.

BrainMaestro · 2018-09-15T20:20:17Z

Hi @Aaronepower can you take another look at it? The output is now more inline with what you posted initially.

--------------------------------------------------------------------------------------------
 Language                         Files        Lines         Code     Comments       Blanks
--------------------------------------------------------------------------------------------
 JSON                           1|23%           1637         1637            0            0
 Markdown                       5|26%           1841         1841            0            0
 Rust                          13|29%           2078         1580          208          290
 Shell                          4|1.8%           128           92            8           28
 SVG                            8|16%           1166         1126            0           40
 TOML                           1|1.2%            84           69            0           15
 YAML                           1|1.1%            80           60            8           12
--------------------------------------------------------------------------------------------
 Total                           33             7014         6405          224          385
--------------------------------------------------------------------------------------------

I tried to reduce the noise a bit. The code isn't so great though

XAMPPRocky · 2018-09-17T16:49:26Z

src/main.rs

+
+    let percent_char_count = 5;
+    // don't show percentage for the total row because that would be pointless
+    if language.lines == total.lines {


Instead of passing in total and performing this if statement, why not call total's print_language with show_percentage as always false.

I still need to perform special formatting for total when show_percentage is true. It's so that the total number of files line up well. You can see the difference in the last two outputs I posted

I think then that there should probably be a seperate function for printing total. Currently this implementation changes a lot of behaviour (such as moving the total around which adds additional allocations in AddAssign) which is not desired.

How would you suggest I do that?

So I've refactored all the printing code into src/cli_utils.rs so you'll need to move your changes to that. I would suggest having a function called print_total that handles all the logic of printing out the total.

I'm not able to rework this now, so I'll pick it up again later when I can. I'll just go ahead and close the PR until then. thanks

@BrainMaestro Okay, thank you again for your PR! I'm going to take some time and write a comment on the issue detailing exact requirements so that if and when you do get back to it you'll have a much clearer sense of the task.

@Aaronepower that sounds great.

I'm going to take some time and write a comment on the issue detailing exact requirements

Hi, was this ever written? I'm interested in trying to (re)implement this. Although I'm new to Rust and open source.

BrainMaestro commented Sep 5, 2018

View reviewed changes

BrainMaestro force-pushed the add-percent-column branch from 9bf00c7 to 531cc6d Compare September 6, 2018 14:57

Add percent to stats

37c8e4d

BrainMaestro force-pushed the add-percent-column branch from 531cc6d to 37c8e4d Compare September 15, 2018 20:16

XAMPPRocky reviewed Sep 17, 2018

View reviewed changes

BrainMaestro closed this Sep 21, 2018

BrainMaestro deleted the add-percent-column branch September 21, 2018 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add percent column #258

Add percent column #258

BrainMaestro commented Sep 5, 2018 •

edited

Loading

BrainMaestro Sep 5, 2018

BrainMaestro commented Sep 5, 2018

BrainMaestro Sep 5, 2018

XAMPPRocky commented Sep 5, 2018

BrainMaestro commented Sep 5, 2018

XAMPPRocky commented Sep 5, 2018

BrainMaestro commented Sep 6, 2018

XAMPPRocky commented Sep 6, 2018

BrainMaestro commented Sep 15, 2018

XAMPPRocky Sep 17, 2018 •

edited

Loading

BrainMaestro Sep 18, 2018

XAMPPRocky Sep 21, 2018

BrainMaestro Sep 21, 2018

XAMPPRocky Sep 21, 2018

BrainMaestro Sep 21, 2018

XAMPPRocky Sep 21, 2018 •

edited

Loading

BrainMaestro Sep 24, 2018

xnzac Dec 31, 2021

Add percent column #258

Add percent column #258

Conversation

BrainMaestro commented Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

BrainMaestro commented Sep 5, 2018

Choose a reason for hiding this comment

XAMPPRocky commented Sep 5, 2018

BrainMaestro commented Sep 5, 2018

XAMPPRocky commented Sep 5, 2018

BrainMaestro commented Sep 6, 2018

XAMPPRocky commented Sep 6, 2018

BrainMaestro commented Sep 15, 2018

XAMPPRocky Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XAMPPRocky Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrainMaestro commented Sep 5, 2018 •

edited

Loading

XAMPPRocky Sep 17, 2018 •

edited

Loading

XAMPPRocky Sep 21, 2018 •

edited

Loading