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 csv-table directive #1030

Merged
merged 9 commits into from
Apr 19, 2024
Merged

✨ Add csv-table directive #1030

merged 9 commits into from
Apr 19, 2024

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Mar 25, 2024

Implements https://docutils.sourceforge.io/docs/ref/rst/directives.html#csv-table-1, addressing need on #189

  • Support URL / file?
  • Support headings?
  • Handle invalid markup

See #893.

@agoose77 agoose77 force-pushed the agoose77/feat-csv-table-directive branch from 2e44155 to a7d138b Compare April 16, 2024 15:21
@agoose77 agoose77 changed the base branch from main to agoose77/feat-nested-parser April 16, 2024 15:21
@agoose77
Copy link
Contributor Author

agoose77 commented Apr 16, 2024

@rowanc1 this PR will probably be easier to use to audit #1091

@agoose77 agoose77 force-pushed the agoose77/feat-csv-table-directive branch from a7d138b to 14ea13e Compare April 16, 2024 15:25
Base automatically changed from agoose77/feat-nested-parser to main April 16, 2024 20:56
@rowanc1
Copy link
Member

rowanc1 commented Apr 16, 2024

@agoose77 there needs to be some fixing of the git history now that #1091 is in! In this case, it is just the last commit.

@agoose77 agoose77 force-pushed the agoose77/feat-csv-table-directive branch from 14ea13e to 419148e Compare April 17, 2024 14:45
@agoose77 agoose77 force-pushed the agoose77/feat-csv-table-directive branch from 419148e to 4b0da69 Compare April 17, 2024 14:46
@agoose77
Copy link
Contributor Author

agoose77 commented Apr 18, 2024

@rowanc1 I've held off adding file/URL support for this pass - I don't think it should block this PR. I'll discuss that on Discord!

The only outstanding challenge is where to add tests ... I'm somewhat happy to skip tests here, as I think those are more end-to-end features that we might need to do another pass of down the road. That might also be a lazy developer's hat.

@agoose77 agoose77 closed this Apr 18, 2024
@agoose77 agoose77 reopened this Apr 18, 2024
@agoose77 agoose77 marked this pull request as ready for review April 18, 2024 11:02
@agoose77 agoose77 requested a review from rowanc1 April 18, 2024 13:38
Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

I think that a small test would be great to have on the directive or main part of the run function (which we will likely need to export later if we are doing a delayed, async load of the CSV in a transform). The test should at least cover the error state of bad CSV. I can add that when I test/export if I get there first!

})),
);
}
const cells = parseCSV(data.body as string, data.options, ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Probably a try catch and errorFile on this if the csv is malformed!

@agoose77
Copy link
Contributor Author

I've made the change to use fileError, but I want to dig in to examples of fileError that seem to pull the nodes for the options, rather than the directive itself, from the pre-AST. Not quite au-fait with that logic yet, but this is an example:

select('mystDirectiveOption[name="tags"]', data.node) ?? data.node

@rowanc1
Copy link
Member

rowanc1 commented Apr 18, 2024

Providing a node with the option reports the correct line number of the option in the error, rather than the directive itself. It falls back to the directive in that case if it can't find the specific option. When we get column numbers and language server support, these will be correctly-placed red-underlines. :)

@agoose77
Copy link
Contributor Author

Oh yes, I'm following the use-case! What I meant was, I wasn't sure of the AST layout of these raw nodes. But, I think I've found it! I'll revise the change.

@rowanc1 rowanc1 changed the title ✨ Add csv-table directive ✨ Add csv-table directive Apr 19, 2024
@rowanc1 rowanc1 mentioned this pull request Apr 19, 2024
2 tasks
@rowanc1 rowanc1 merged commit a3e3aa0 into main Apr 19, 2024
5 checks passed
@rowanc1 rowanc1 deleted the agoose77/feat-csv-table-directive branch April 19, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants