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

move parameter tables to CSV and read into MD #501

Merged

Conversation

jimustafa
Copy link
Contributor

This PR moves the parameter tables from the markdown source to separate CSV files and uses mkdocs-table-reader-plugin to import the tables from the CSV files.

One advantage of this approach is that all the parameters definitions are in a single location, separate from the documentation text. The CSV form is a bit easier to edit and manipulate without dealing with the markdown table syntax. Additionally, the CSV files can be used for other purposes, such as a VS Code extension that I am playing around with to create snippets for writing WIN files, with all the parameters documented.

If the @wannier-developers would like to continue with this approach, I will complete the move of the remaining parameter tables to CSV.

@jimustafa jimustafa marked this pull request as draft May 8, 2024 06:36
@qiaojunfeng
Copy link
Collaborator

Thanks Jamal, this is great, please go ahead!

We can reuse the CSV in many other places, e.g. I have a tree-sitter grammar to parse the syntax of win file https://github.com/qiaojunfeng/tree-sitter-wannier90 , with the CSV we can have more accurate syntax highlighting.

A small issue is the footnote, e.g. after the atoms_cart or atoms_frac, it might be confusing if someone just loads the CSV and treats the raw string including stars as a valid parameter. If there is no better solution, maybe we can move the footnotes to the Description column of each row, then we can always blindly load the parameter names from CSV in any language without the need to take care of stars.

@jimustafa jimustafa force-pushed the revamp-docs-parameter-tables branch from fe54a7f to 64f6e08 Compare May 9, 2024 04:07
@jimustafa
Copy link
Contributor Author

I moved all the parameters to a single XML file and created a script to regenerate the CSV parameter tables when building the documentation (done with MkDocs on_pre_build hook). With XML, we should have more flexibility in documenting the parameters and it could be more useful for other applications. In the case of parameters with footnotes, we could maybe add an attribute like "note=*" or something.

The XML format is not set in stone and any feedback would be appreciated. It is not clear to me when it is best to use elements vs attributes. Some additional information that we could add:

  • parameter default value
  • the set of choices for parameters that are strings
  • whether the parameter is an array and maybe even what shape

Thanks for pointing me to qiaojunfeng/tree-sitter-wannier90! I had not even heard of Tree-sitter! Always interested to check out something new.

@jimustafa jimustafa force-pushed the revamp-docs-parameter-tables branch from 64f6e08 to b5319c2 Compare May 26, 2024 20:26
@jimustafa jimustafa marked this pull request as ready for review May 26, 2024 20:28
@jimustafa
Copy link
Contributor Author

The CSV files could be generated on-the-fly using MkDocs hooks, but when I tried, I kept getting a looping build, probably something to do with generating the CSV files using generate-parameter-tables.py retriggering the build. There may be better locations for the parameters.xml file, generation script, and resulting CSV files to prevent this...

@jimustafa
Copy link
Contributor Author

Some more parameters were added to the docs after this PR was opened, which caused some conflicts. The new parameters were added to parameters.xml and to the associated CSV files used to generate the parameter tables.

Also, fprettify was transferred to @fortran-lang, so .pre-commit-config.yaml needed updating. If this PR is not merged, then the fix would need to be applied separately.

@qiaojunfeng
Copy link
Collaborator

Thanks Jamal!

@qiaojunfeng qiaojunfeng merged commit 8059cc5 into wannier-developers:develop Sep 27, 2024
5 checks passed
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