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

Feat/org table rows #191

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Feat/org table rows #191

wants to merge 8 commits into from

Conversation

nobiot
Copy link
Owner

@nobiot nobiot commented May 28, 2023

This is continuation of the discussion thread in PR #190.

@llcc

GitHub is very slow to respond on my end. I will come back to add more information. But just quickly I wanted to let you know about the approach I was talking about in #190

I don't know how to continue with you branch and PR so I opened a new one -- I have done this in the hope that showing code communicates the idea better than describing it with words. Please let me know if this works for you. I am hoping that you'd be able to continue with your with your original motivation -- either via this PR or yours.

The summary of what I have done is as follows:

  • Refactored the way Org content filter works
  • Created a new file dedicated to org-table-row "filtering" and implemented a "prototype" to just show how the logic would work
  • Created a new test file for org-table-rows

Add content-filter-org-functions.
`run-hook-with-args' (and other `run-hook' functions) does not seem to
be designed to receive a returned value from each function. What I would
need is a way to apply each filter function one by one to the parse
tree (obj). `dolist' can be made do this.
@llcc
Copy link

llcc commented May 29, 2023

This is continuation of the discussion thread in PR #190.

@nobiot Thanks for bring this up.

  • Created a new file dedicated to org-table-row "filtering" and implemented a "prototype" to just show how the logic would work

I personally support the approach proposed in my PR, as it is much more flexible for any element beyond just tables, as previously mentioned. The remaining challenge is to address the live editing for this specific :line format.

Could you please provide additional details or further clarification regarding the intention behind your proposal?

@nobiot
Copy link
Owner Author

nobiot commented May 30, 2023

@llcc Of course I will be happy to add more detail and my motivation. Let me know if you wish to know more -- I am hoping you could bring this PR to the state where it can be merged. Otherwise, I'd be happy to just merge my refactoring and leave this PR on hold.

Motivation

The motivation for the code I include in this PR is to support your use cases for Org table rows and later Org list. I don't have strong need for them at the moment. You are the only reason and your use cases seem useful to others :-)

Please take my PR as a suggestion. I will merge my refactoring even if you do not use this approach. I am happy to see your enhancement to the :lines syntax. You can keep it in your local environment and use it as your personal customization.

Having said this, I am not able to merge your approach into org-transclusion codebase. The reason is simple: org-transclusion-src-lines is a library designed for text files that are not Org. :lines property is not meant to be used for Org files. You could use it for an Org file, but then the package will treat it just as a normal text file.

For Org files, we have a mechanism for "filtering" Org elements with using the org-element package, the built-in Org parser.

What this PR includes

This PR may seem to includes a lot, but there are fundamentally only two things:

  1. Refactoring of Org filtering mechanism in org-transclusion.el

  2. Sample code as a prototype of org-table-row support with using the refactored Org filtering mechanism (with test data for manual testing)

If you take this approach, all you need to look at is the org-transclusion-org-table-rows.el file, especially org-transclusion-keyword-value-org-table-rows and org-transclusion-content-filter-org-table-rows-function

In order to support filtering of table rows, I needed to refactor the filtering mechanism for Org files in general. This is something I have postponed. I thought it would be a good opportunity to do this.

Some benefits of this filtering approach

I believe there are some merits to the approach if you can stay with a little longer and hear me out.

  1. It takes care of many things automatically (without additional code):

    • Support for #+name from Org mode with file:::

    • The boundary of the end of table, including the #+TBFM line

    • Internal links (fuzzy links) for a table name within the same buffer when I merge the feature/org-internal-links branch

    • No need to worry about live-sync support when you code extension (regression testing is needed, but in general live-sync skips the filtering to mirror the source Org element as it is)

  2. Built-in org-element is flexible

    • For example, the sample code is very simple and yet expressive (to me)
      This part is the key. As org-element-map here loops each table-row, I just simply add a counter and remove when the counter is 3 (the 3rd row) by calling (org-element-extract-element row)) -- org-element-extract-element removes the element from the parse tree as a side effect (you can read this in its documentation string).

    • You could also look to implement syntax for "exclude rows"

      :table-rows 1..20 :table-rows-exclude 10,15..19 (I am suggesting the ".." notation might be more appropriate for a range of tables rows as it is the same as org-table.el):

      Include rows 1 from 20 (inclusive) except for 10 and 15-19 (inclusive)

    • Potentially it can be enhanced to support an arbitrary PREDICATE function to do something like "Transclude only the rows where column 2 is great than 100" (future enhancement ideas only at this stage)

@llcc
Copy link

llcc commented May 31, 2023

@nobiot Thanks for your explanation. I'll reply you later here.

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.

2 participants