-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Rework simple and multilevel counters #1541
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this all makes sense.
Re tests: I would suggest Busted based unit tests rather than full page layout regression tests since we're after the math here more than the resulting typesetting.
Re class scope: The exporting stuff is mostly to support v0.13 classes and packages that didn't have another way to access functions from other packages. Now in v0.14 we do and it's possible we could eventually deprecate those exports entirely. For now I would leave the ones we have but encourage not adding any more unless absolutely necessary. Packages can now access other packages methods directly via self.class.packages["other-package"]:method
. We could possibly even make that more ergonomic, but for now lets keep an eye on what we need it for so we know what to do with the existing export system.
The one caveat is exporting functions make it easy for them to access the class as self
rather than the package, which might be a good reason to keep it for certain senarios.
e691329
to
bfe4d81
Compare
Updated:
|
Hmm. Not sure what I did here, it was unintended at best^^ Maybe the rebasing? |
I had commented on a specific line of code in the review. When you updated the PR tweaking that line it dismissed my review on the assumption that with updates I should review it again and see if what I commented was fixed. No problems here, just the way GitHub is supposed to work.
Git commits themselves have two basic properties: author and committer. Usually those are the same, but sometimes when you do things like rebase other people' commits you might become the committer while they stay as the author. More recently GitHub and others (notably the 800 pound gorilla code review platform Gerrit) decided the 2 field system was restrictive, and started parsing the content of commit messages for more structured information. Editing these fields is just a matter of typing the expected text into the body of a commit message. You can even use multiples. Some semi-standardized fields I know of:
If GitHub detects a matching name/email that it knows about (probably just email?) it will show some UI goodies like linking their profile when relevant. They are not heavily used, but sometimes when somebody posts code in an issue but doesn't send a PR I find it nice to credit them. I usually just lookup the name/email pair they use on commits elsewhere and drop it in if I copy code from someone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
Yup that's exactly what I was thinking. Also need to make sure that sort of layout works for 3rd party packages so external repos can be tested the same way without too much fuss. I'm guessing the easiest way to do this will be with |
When setting/incrementing a level far below the current level, previous level are set to 0 rather than the value of the previous valid level, e.g. if setting 1.3 at level 5, we expect 1.3.0.0.1 rather than 1.3.3.3.1.
bfe4d81
to
71522ce
Compare
I rewrote the commits (via advanced |
Closes #1536 - I suffered a bit on this one, so these are at least some proposals for review and scrutiny...
Things proposed here:
SU.cast()
,SU.boolean()
checks, etc.noleadingzeros=true
to multilevel counter formatting, to possibly skip leading zerosset-multilevel-counter
\increment-counter[set-to=...]
: it was not documented, and an increment that does a set is weird. We already have\set-counter
for that, let's keep things simple, no?\show-counter[display=...]
and\show-multilevel-counter[display=...]
: these are actually altering the counter, this does not seem expected on a "show" command which shouldn't have side effects, and we can already change the display format in a much more understandable way with the "set" or "increment" commands. Again, let's keep simple and clear. A "show" just shows without side-effects.reset
undocumented option onincrement-multilevel-counter
but it was pretty broken (it could not be used from SIL-language, as it was lacking a cast) and what it did when the passedlevel
is greater than the current level did not seem right.Things not yet done, for the record:
Another unaddressed point I'd like to raise here: it doesn't seem logical to me to export some functions at class level (e.g.
class:getCounter
, while some aren't (well, still are, but deprecated, e.g.class:formatCounter
). There's some hard-to-understand asymmetry.