Morgan Smith <[email protected]> writes: > Am I allowed to change the subject line? Does that mess things up? I > want the subject to be "Rewrite 'org-clock-timestamps-change'".
Feel free. If this breaks bark, we should fix bark, not the mailing list. > Ihor Radchenko <[email protected]> writes: > >> Morgan Smith <[email protected]> writes: >> >>> >>> If we want to go through all the org functions like this to make sure >>> the interactive specs are correct and we aren't calling >>> `prefix-numeric-value' unnecessarily, then there are probably about 15 >>> functions we need to change. >> >> I just referred to this particular function you are changing in the >> patch. It has exactly two callers that declare their interactive spec as >> "raw". We can instead change those callers to pass numeric value. >> Does it make sense? > > The issue here has nothing to do with difficulty and everything to do > with scope. The commit is specifically "rewrite > 'org-clock-timestamps-change'". Sneaking those two changes to other > functions into that commit does not follow the rule of "one commit one > change". So you are asking me to add an additional commit of fixing up > the interactive spec. But now I've got a commit that is literally 2 > lines which is also not good as we don't want many tiny commits in our > project. This is why I've decided that in order to do what you've said > requires me to go through all the interactive declarations. Hmm. I look at this differently. We are fixing a number of bugs in org-clock-timestamp-change. Those fixes revealed that some callers of org-clock-timestamp-change do not call it according to the docstring, which worked in the past by chance (and with strange workarounds further in the call chain). So, we need several fixes in a row. Either squashed or as separate commits. Also, I do not see tiny commit as a problem. If a commit is fixing a real problem, it is worth it, even if is it a one letter change. > Well you've done it. Now I'm going through all the interactive > declarations and fixing things. That was not my intention. > So can we simply approve these changes and I'll start a new thread in a > bit? I mostly want to know what you think of "Add match groups to > `org-element-clock-line-re'" If you feel strongly about this, sure. I honestly thought this to be a small supplementary we can do alongside the patch. Clearly, you see it differently, so I will not insist. The patch is already an improvement anyway. Match groups are ok. If we want to be super-careful, we may announce the change in news or rewrite it to have "no group" version as old variable and introduce a new with groups, but I think the change you proposed has low chance to break things and should be ok as is. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
