tld: comprehensive rewrite #1939
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
List is cached in `bot.memory` at runtime, and persisted to a plugin value in `bot.db` at shutdown. If a cached list exists, it is loaded into `bot.memory` at startup. The list is updated automatically once the data is 7 days old.
Replaced those horrid (and ugly) regex patterns with a bare-bones HTML parser, an in-memory lookup dictionary, and dynamic generation of the output line depending on what fields are available. Surely there are some remaining edge cases (though I threw a number of different TLDs at this and knocked out some already), but this is an absolutely MASSIVE improvement in maintainability IMO. Uses the same caching logic as for the IANA TLD list, so any given Sopel instance will only hit Wikipedia at most once a week or so.
Also moved the data-processing step inside the parser object, because having a helper function for that at the module level didn't make sense.
|
Update: I was gathering examples of the new output and found one table that gets parsed wrong. Will need to investigate that one. A quick look at the HTML just now didn't reveal any obvious structural difference, and I unfortunately can't spend any more time on this until Friday evening (9/18 in central US, for our non-American project members) at the earliest. Still feels good to have mostly done, even if a gremlin or two remain lurking. |
Exirel
approved these changes
Sep 22, 2020
|
I'll just merge this as-is and follow up with another PR to fix that table later (or an issue, if I don't get to it soon). Like @Exirel said, "it's working fine", and I agree that one table misbehaving a bit shouldn't block the vast improvement that is the rest of this PR. |
This was referenced Sep 24, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In short, this fixes Sopel being unable to find known-valid TLDs. But it also makes things better than before. No more ugly, unmaintainable regex!
HTMLParserBoth the IANA and Wikipedia data sources are cached locally, persisted/restored to/from the bot's database on shutdown/startup, and periodically updated by a plugin job (minimum age 7 days).
I think about the only thing I didn't do was add example-tests. But I haven't decided against doing so. I've just not gotten to it.
Checklist
make qa(runsmake qualityandmake test)See also
This might be sufficient to consider #1648 as "done". I've been unable to find any viable alternative data source that actually provides the information contained in Wikipedia's tables—even the basics like whether a TLD allows second-level registrations or supports DNSSEC/IDN/etc.