accept md and rmd files; refactor align, shrink, and calc_column_stats to work on address/visual range #52

carywreams · 2024-06-05T15:20:45Z

plugin/rainbow_csv.vim converts RainbowAlign and RainbowShrink to use -range=%
autoload/rainbow_csv.vim#csv_align
- accepts either address or visual range
- changes operational loops to have specific start/stop lines
- emits error if an entire markdown or rmd file is submitted
autoload/rainbow_csv.vim#csv_shrink
- accepts either address or visual range
- changes operational loops to have specific start/stop lines
- emits error if an entire markdown or rmd file is submitted
autoload/rainbow_csv.vim#calc_column_stats
- accepts two new parameters, firstline and lastline
- changes operational loops to have specific start/stop lines
autoload/rainbow_csv.vim#s:named_syntax_map adds markdown and rmd filetype entries
proposed README.md changes
proposed doc/rainbow_csv.txt changes

… of lines

…csv_align to use either address or visual range

…tance as well as requirements for ranges when using align and shrink functions in those filetypes

carywreams · 2024-06-05T17:10:15Z

addresses #51

mechatroner · 2024-06-06T01:52:40Z

Awesome! Thank you! Looking at it

mechatroner

Thank you for sending this PR!
There are some adjustments that have to be made before I can merge it.

mechatroner · 2024-06-06T01:55:31Z

README.md

Please revert all whitespace-related changes, they are meaningful in markdown - removing them breaks the doc formatting when it is rendered.

everything else good to go. hunting down where, in my vim settings, I have trailing whitespaces getting trimmed automatically.

As I've never seen it before, what's the effect of trailing whitespaces on markdown documents ? or, what kind of rendering is being done that relies on the trailing whitespaces ?

As soon as I've addressed that, I will resubmit.

Two whitespaces at the end create a linebreak in GFM (Github-Flavored Markdown). There are multiple competing markdown dialects actually: https://en.wikipedia.org/wiki/Markdown but since this repo is hosted on github we should obviously follow the github spec.

mechatroner · 2024-06-06T01:57:41Z

README.md

+|csv_pipe        |&#124; (pipe)   |            |                                                      |
+|rfc_csv         |, (comma)       |            |Same as "csv" but allows multiline fields             |
+|rfc_semicolon   |; (semicolon)   |            |Same as "csv_semicolon" but allows multiline fields   |
+|markdown        |&#124; (pipe)   |.md         |align and shrink functions only operate on ranges     |


I don't think we should add markdown filetypes to this list since they are being handled in a special way only for the purpose of the Align/Shrink procedures. Instead, you can write more about this special case in the docs for Align and Shrink commands. This is because the main feature of the extension is highlighting and users could wrongly assume that Rainbow CSV somehow adjusts markdown highlighting too.

mechatroner · 2024-06-06T01:59:46Z

README.md

+Align CSV columns with whitespaces.
+Don't run this command if you treat leading and trailing whitespaces in fields as part of the data.
+You can edit aligned CSV file in Vim column-edit mode (Ctrl+v).
+In markdown and r-markdown files, this function requires a range of addresses or a visual range selection.


I would extend this comment by adding that in markdown files the RainbowAlign command uses pipe separator by default and only works on ranges and/or visual range selection or something like this.

mechatroner · 2024-06-06T02:01:28Z

autoload/rainbow_csv.vim

@@ -17,7 +17,7 @@ let s:system_python_interpreter = ''

 let s:magic_chars = '^*$.~/[]\'

-let s:named_syntax_map = {'csv': [',', 'quoted', ''], 'csv_semicolon': [';', 'quoted', ''], 'tsv': ["\t", 'simple', ''], 'csv_pipe': ['|', 'simple', ''], 'csv_whitespace': [" ", 'whitespace', ''], 'rfc_csv': [',', 'quoted_rfc', ''], 'rfc_semicolon': [';', 'quoted_rfc', '']}
+let s:named_syntax_map = {'csv': [',', 'quoted', ''], 'csv_semicolon': [';', 'quoted', ''], 'tsv': ["\t", 'simple', ''], 'csv_pipe': ['|', 'simple', ''], 'csv_whitespace': [" ", 'whitespace', ''], 'rfc_csv': [',', 'quoted_rfc', ''], 'rfc_semicolon': [';', 'quoted_rfc', ''], 'markdown':['|', 'simple', ''], 'rmd':['|', 'simple', '']}


Please don't change this list since it can be used by other logic.

mechatroner · 2024-06-06T02:06:49Z

autoload/rainbow_csv.vim

    " The first (statistic) pass of the function takes about 40% of runtime, the second (actual align) pass around 60% of runtime.
    " Numeric-aware logic by itself adds about 50% runtime compared to the basic string-based field width alignment
    " If there are lot of numeric columns this can additionally increase runtime by another 50% or more.
-    let show_progress_bar = wordcount()['bytes'] > 200000
+    let show_progress_bar = (l:lastline - l:firstline) > 200000


Please revert, you replaced byte count with line count

mechatroner · 2024-06-06T02:16:24Z

autoload/rainbow_csv.vim

    " The first (statistic) pass of the function takes about 40% of runtime, the second (actual align) pass around 60% of runtime.
    " Numeric-aware logic by itself adds about 50% runtime compared to the basic string-based field width alignment
    " If there are lot of numeric columns this can additionally increase runtime by another 50% or more.
-    let show_progress_bar = wordcount()['bytes'] > 200000
+    let show_progress_bar = (l:lastline - l:firstline) > 200000
    let [delim, policy, comment_prefix] = rainbow_csv#get_current_dialect()
    if policy == 'monocolumn'
        echoerr "RainbowAlign is available only for highlighted CSV files"


Instead of immediately returning you can adjust delim and policy to '|' and simple for markdown files - this would be the special case at Align level only. same for the shrink. You can also check here that range is provided instead of checking it below.

mechatroner · 2024-06-06T02:17:56Z

autoload/rainbow_csv.vim

@@ -835,25 +834,35 @@ func! rainbow_csv#csv_align()
        echoerr 'RainbowAlign not available for "rfc_csv" filetypes, consider using "csv" instead'
        return
    endif
-    let lastLineNo = line("$")
+
+    if (l:firstline == 1 && l:lastline == line("$") && (&ft == 'markdown' || &ft == 'rmd'))


Please move this check higher, see my other comment.

mechatroner · 2024-06-06T02:18:47Z

autoload/rainbow_csv.vim

-
-func! rainbow_csv#csv_align()
+func! rainbow_csv#csv_align() range
+    let l:firstline = a:firstline


Please don't use l: prefix for consistency with other code.

mechatroner · 2024-06-06T02:20:33Z

autoload/rainbow_csv.vim

@@ -883,12 +892,13 @@ func! rainbow_csv#csv_align()
        let is_first_line = 0
    endfor
    if !has_edit
-        echoerr "File is already aligned"
+        echoerr "Range is already aligned"


Please keep the original comment since it is a more common use case, and seeing the range might be confusing.

mechatroner · 2024-06-06T02:21:44Z

autoload/rainbow_csv.vim

    endif
 endfunc

-
-func! rainbow_csv#csv_shrink()
+func! rainbow_csv#csv_shrink() range


All my comments regarding the align function also apply to Shrink function.

done.
moved the show_progress_bar setting to location similar to that found in _align, so that the exception processing block for md/rmd files could remain in a single location, as in _align.

…ce to README.md

…ce to autoload/.vim and doc/.txt

carywreams · 2024-06-06T14:05:16Z

I believe that will handle the current set of comments. Let me know if there's more we can do.

mechatroner · 2024-06-07T00:49:41Z

autoload/rainbow_csv.vim

-    let is_first_line = 1
-    for linenum in range(1, lastLineNo)
+    let lastLineNo = a:last_line
+    let is_first_line = a:first_line


This is a boolean var that affects how numeric-only columns with a non-numeric header are aligned.

sorry about that . complete miss on my part while converting to ranges

mechatroner · 2024-06-07T00:51:07Z

autoload/rainbow_csv.vim

    " The first (statistic) pass of the function takes about 40% of runtime, the second (actual align) pass around 60% of runtime.
    " Numeric-aware logic by itself adds about 50% runtime compared to the basic string-based field width alignment
    " If there are lot of numeric columns this can additionally increase runtime by another 50% or more.
    let show_progress_bar = wordcount()['bytes'] > 200000
    let [delim, policy, comment_prefix] = rainbow_csv#get_current_dialect()
+    if (&ft == 'markdown' || &ft == 'rmd')


This condition should only override "monocolumn" policy, otherwise get_current_dialect() policy should have priority

Actually, this might not be a big deal, I can fix it myself, later, so you can leave it as is if you want. "monocolumn" is just a fall back which means that no better dialect was found.

after reading through the rest of the file, wondering if &ft is correct choice over &syntax. Input?

mechatroner · 2024-06-07T00:52:17Z

autoload/rainbow_csv.vim

@@ -852,8 +865,8 @@ func! rainbow_csv#csv_align()
        return
    endif
    let has_edit = 0
-    let is_first_line = 1
-    for linenum in range(1, lastLineNo)
+    let is_first_line = first_line


Please fix, this is a boolean variable.

mechatroner · 2024-06-07T00:55:51Z

autoload/rainbow_csv.vim

    " The first (statistic) pass of the function takes about 40% of runtime, the second (actual align) pass around 60% of runtime.
    " Numeric-aware logic by itself adds about 50% runtime compared to the basic string-based field width alignment
    " If there are lot of numeric columns this can additionally increase runtime by another 50% or more.
    let show_progress_bar = wordcount()['bytes'] > 200000
    let [delim, policy, comment_prefix] = rainbow_csv#get_current_dialect()
+    if (&ft == 'markdown' || &ft == 'rmd')
+        if (first_line == 1 && last_line == line("$"))


I think this check might be unreliable, please use this idiom instead: https://vi.stackexchange.com/questions/24793/how-to-check-whether-a-command-is-run-with-range-or-not

Actually, it is fine to keep your implementation if you prefer, since this is just a warning which won't be able to produce a serious bug.

mechatroner · 2024-06-07T01:03:26Z

README.md

Two whitespaces at the end create a linebreak in GFM (Github-Flavored Markdown). There are multiple competing markdown dialects actually: https://en.wikipedia.org/wiki/Markdown but since this repo is hosted on github we should obviously follow the github spec.

+ is_last_line returned to treatment as bool + range-provided checks converted to use <range> + predicates exception for markdown/rmd files on current dialect = monocolumn for both shrink and align functions + adds maps of exception filetypes; used in both shink and align functions; other variations of markdown may be supported in future

mechatroner · 2024-06-09T00:41:01Z

Thank you for your adjustments! I will take it from here, would still need to test it and maybe make some additional changes. I currently have some other competing priorities but when I have some free time on my hands I will finalize the integration. Stay tuned!

carywreams added 3 commits June 4, 2024 14:51

issue mechatroner#51: initial conversion of csv_align to accept range…

6a58ed8

… of lines

issue mechatroner#51: range-enabled csv_shrink; converts csv_shrink, …

074b1fa

…csv_align to use either address or visual range

issue mechatroner#51: adds documentation of md and rmd filetype accep…

76963cc

…tance as well as requirements for ranges when using align and shrink functions in those filetypes

mechatroner requested changes Jun 6, 2024

View reviewed changes

carywreams added 3 commits June 6, 2024 08:17

w/r/t mechatroner#51, addressing PR mechatroner#52 comments and concerns

c5c52c5

w/r/t mechatroner#51, addressing PR mechatroner#52; restores whitespa…

0b618d1

…ce to README.md

w/r/t mechatroner#51, addressing PR mechatroner#52; restores whitespa…

6fcc0c7

…ce to autoload/.vim and doc/.txt

mechatroner requested changes Jun 7, 2024

View reviewed changes

accept md and rmd files; refactor align, shrink, and calc_column_stats to work on address/visual range #52

Are you sure you want to change the base?

accept md and rmd files; refactor align, shrink, and calc_column_stats to work on address/visual range #52

Conversation

carywreams commented Jun 5, 2024

carywreams commented Jun 5, 2024

mechatroner commented Jun 6, 2024

mechatroner left a 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

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

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

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

carywreams commented Jun 6, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mechatroner commented Jun 9, 2024