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

Added footnote support and improve balancing logic for parallel typesetting. #2212

Closed
wants to merge 0 commits into from

Conversation

no-vici
Copy link

@no-vici no-vici commented Jan 12, 2025

Introduced some features for footnote handling, improved frame alignment logic, and support for customisable glue and spacing.

@no-vici no-vici requested a review from alerque as a code owner January 12, 2025 12:07
@no-vici no-vici changed the title Update init.lua Added footnote support and improve balancing logic for parallel typesetting. Jan 12, 2025
@alerque
Copy link
Member

alerque commented Jan 12, 2025

Wow thanks for jumping in and contributing.

I don't have time to give this a proper review this moment, but I certainly will. Just some preliminary feedback (and these are all things we can help fix and address, just FYI):

  1. You seem to have your editor set to override existing indentation with tabs. We're using 3-space indent. You might want to enable an EditorConfig aware extension in your editor so it automatically adjusts to the right coding style for projects.

  2. The Lua styling (including the above spacing fixes) are easily fixed after the fact using stylua. It will deterministically reformat all the Lua code in the project, so don't bother fixing it manually, we'll just run that and get this fixed up.

  3. Commit messages need to follow Conventional Commits guidelines. You can look at the history for ideas of the scope names we use. Again these can be fixed up after the fact, just a note for future reference.

  4. We tend to prefer granular commits. Do small increments and label them as features, fixes, refactors, docs, etc. One commit per task scope. We can work on splitting this up, and I can help do that while reviewing it, but as you work just keep that in mind, it is usually easier to keep more granular commits than you may end up wanting and squash them than it is to split up later if you do too big, so err or the side of many commits when in doubt.

classes/diglot.lua Outdated Show resolved Hide resolved
@alerque alerque added enhancement Software improvement or feature request modules:packages Issue relates to core or 3rd party packages labels Jan 12, 2025
@alerque alerque added this to the v0.15.10 milestone Jan 12, 2025
@alerque
Copy link
Member

alerque commented Jan 12, 2025

I just tried to fix some of the issues noted, but ran into one more thing: when you open PRs, it helps a lot if you start a new branch on your fork before opening the PR. That way you can allow us (maintainers) to contribute to the PR. As it is this is opened against your own forks master branch, which you cannot let us push to.

I just pushed my modifications of your commits to this branch. You can reset your branch to the latest commit there, or run stylua and split up commits yourself, or close this PR and open one on a branch so I can just push directly to it, or whatever....

@no-vici
Copy link
Author

no-vici commented Jan 13, 2025

Wow thanks for jumping in and contributing.

I don't have time to give this a proper review this moment, but I certainly will. Just some preliminary feedback (and these are all things we can help fix and address, just FYI):

  1. You seem to have your editor set to override existing indentation with tabs. We're using 3-space indent. You might want to enable an EditorConfig aware extension in your editor so it automatically adjusts to the right coding style for projects.
  2. The Lua styling (including the above spacing fixes) are easily fixed after the fact using stylua. It will deterministically reformat all the Lua code in the project, so don't bother fixing it manually, we'll just run that and get this fixed up.
  3. Commit messages need to follow Conventional Commits guidelines. You can look at the history for ideas of the scope names we use. Again these can be fixed up after the fact, just a note for future reference.
  4. We tend to prefer granular commits. Do small increments and label them as features, fixes, refactors, docs, etc. One commit per task scope. We can work on splitting this up, and I can help do that while reviewing it, but as you work just keep that in mind, it is usually easier to keep more granular commits than you may end up wanting and squash them than it is to split up later if you do too big, so err or the side of many commits when in doubt.

Thank you for guiding me on how to contribute to SILE. I didn't actually know what I was doing, after watching some YouTube guides and just trying things out.

I will learn everything and follow the guidelines.

Thank you for your time and instruction.

Grateful as always.

@alerque
Copy link
Member

alerque commented Jan 14, 2025

In case you are out there looking for this incantation, you can start a new branch from where you left off with this:

git branch -c add-footnotes-in-parallel 5fe88b2

Then push and open a new PR from that branch, and I can help adjust it with some of the other work I did cleaning this up.

@no-vici
Copy link
Author

no-vici commented Jan 15, 2025

In case you are out there looking for this incantation, you can start a new branch from where you left off with this:

git branch -c add-footnotes-in-parallel 5fe88b2

Then push and open a new PR from that branch, and I can help adjust it with some of the other work I did cleaning this up.

You could read my mind, sir.

I'm reading the Git Tutorial written by Dr. Yihsiang Liow and starting from scratch.

I am so sorry for wasting too much of your time for such as trivial things, but grateful for your guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Software improvement or feature request modules:packages Issue relates to core or 3rd party packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants