-
Notifications
You must be signed in to change notification settings - Fork 125
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
Improve y-029/y-031/y-032, add y-031 thru y-033 tests #701
Conversation
Great work! Can we merge y-029 and y-032? They do very similar things, would it make sense to just make it one test? |
I had an issue three-fourths written to suggest exactly that, and changed my mind, because I didn't know if it was going to be OK to be either more liberal in exclusions (like y-032) and possibly miss more, or more restrictive (like y-029) and possibly have more false positives. So, I agree they can/should be combined. Let me do some testing and I'll get back with some data on which direction we might want to go. |
OK, I remember now; I actually did a little bit of testing, and that was the real reason I aborted my proposal. Too many things happened since then. Combining both into the more restrictive y-032 (which looks for text both before and after, while y-029 only checks after, and only a single letter) doesn't work for Keeping y-029 as it is but only for em, and using y-032 for any i, not just epub:type i's. If we remove the New false positives (on a couple of them I don't know whether they're OK or not): New errors caught: So, up to you if you think the tradeoff is worth it. If so, let me know and I'll make the change. |
Meh, so many false positives just to catch three new errors isn't worth it. Let's leave it then. But can you fix the ones that did come up with PRs? |
Yep, I agree. |
Yes you can just go ahead and fix them yourself, thanks! |
Done. |
With this, the tests for lint typos (y-xxx) are complete. At this rate we should be done with all of the lint tests around 2027. :) I think I'm going to take a little break from tests and try to actually read something, or at least work on something to read.
Changes to lint:
y-029—since y-032 is checking for text on italics with epub:types, then y-029 doesn't need to. I added an exclusion for italics with epub:type.
y-031 (all on second re:test)
\b
on each of the he/she/I. Since we're checking for whitespace immediately before, the \b isn't needed (the whitespace already guarantees it).\b
behind it, so it was matching anything that began with I, rather than I itself. I added the '\b'.!
and?
. I added them.y-032—As mentioned in an issue, this is testing for an italic with epub:type immediately followed by text, but it excludes (e|es|er). However, again, there is no
\b
following, so it actually excludes anything starting with any of those three. Although it's possible those were the intentions, I thought it was unlikely, so I went ahead and added it. If that's wrong, it's easy enough to remove.I ran this lint on the corpus before submitting. There were no new false positives, and the changes to y-031 will eliminate six ignore entries.