Skip to content

Refactor creation of puz files and add optional PUZv2 support#204

Open
lmfont wants to merge 338 commits into
thisisparker:mainfrom
lmfont:refactor-puz-use-and-support-v2
Open

Refactor creation of puz files and add optional PUZv2 support#204
lmfont wants to merge 338 commits into
thisisparker:mainfrom
lmfont:refactor-puz-use-and-support-v2

Conversation

@lmfont

@lmfont lmfont commented Aug 6, 2024

Copy link
Copy Markdown
Contributor

Most of the downloaders follow a similar pattern of creating a puzzle file with puz.Puzzle() in their parse_xword method and returning this puzzle file to the controller. This refactor changes this to make puzzle files attributes of the BaseDownloader class, which the downloaders then use as needed.

This has several benefits:

  • The Puzzle interface is now abstract, from the point of view of the downloaders. None of them imports puz any more. This enables a potential future PR to allow swapping out puz for a compatible implementation, e.g. one that would create files in a different format like .ipuz (libipuz).

  • As part of this PR, we add a flag -1 --v1 that creates PUZ v1.0 files, and add (and make default) support for the 2.0 version. Because PUZv2 supports UTF-8, we don't have to strip out e.g. emojis, which are becoming increasingly common in online crosswords. I've tested the results and Gnome Crosswords can show emoji puzzles out of the box after this PR.

lmfont and others added 9 commits June 10, 2024 21:50
The attribute previously used to find puzzle links changed. Rather
than update it to a new one, take the first link in the page matching
the puzzle URL format, as this should be more reliable.

Fixes thisisparker#190.
* wsj: disable downloader command and url matching function
* readme: remove wsj from supported sites 😭
* workflows: remove WSJ tests from status check
* update lxml requirement from 4.9.2 to 5.1.0 (thisisparker#185)

lxml 4.9.2 does not have precompiled wheels available for Python 3.12 (see https://pypi.org/project/lxml/4.9.2/#files), whereas lxml 4.9.3 and newer do. This commit updates the requirement for lxml to 5.1.0 (the latest available version) to allow for installing xword-dl locally under Python 3.12 without having to compile lxml.

* requirements: bump versions on each dependency

---------

Co-authored-by: minicircle <23437981+minicircle@users.noreply.github.com>
* puzzmo: add puzzmo downloader
* readme: list puzzmo support
* workflow: add puzzmo test
* add downloader for daily pop puzzles
* readme: add daily pop support
* workflow: add daily pop to tests
@lmfont

lmfont commented Aug 6, 2024

Copy link
Copy Markdown
Contributor Author

There are a couple of things that need looking at, e.g. I don't really understand what's up with the rebus table encoding or whether that is impacted by this PR.

There are at least two other ways you could do this refactor. If you'd prefer one of these, I'm happy to redo it that way.

  • Have the controller make a Puzzle and send it to parse_xword rather than having it as a long-lived attribute of BaseDownloader.
  • Make our own abstract Puzzle class in a separate file, which could import and use puz.Puzzle or some other library (possibly, in the future). Individual downloaders would import from this file rather than puz directly, but would continue to use the resulting Puzzle object as they do now.

@lmfont

lmfont commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

I've privately rebased this PR to apply cleanly on top of #205 and I think that's the order it makes sense to go in, if you're going to consider both.

…sparker#231)

* add puzzmo big to supported outlets
* add search-by-date and search-by-url to puzzmo downloader
* setup: update min python version because of zoneinfo in puzzmo downloader
* workflows: add checks for new puzzmo methods
* readme: add puzzmo and puzzmo big search methods
* bump version number for release
* bump dependencies for release
lmfont and others added 2 commits May 30, 2025 12:06
…arker#205)

* Fix import of urllib.parse module

Many files import urllib, but this is a package that contains the module
`urllib.parse`. This works, when it does (I haven't checked every case)
because `requests` is also imported, and it imports the module
correctly, which makes it available to xword-dl under the urllib
namespace.

* Fix reference to undefined setting

Because of the way the settings dict is populated, if the preserve_html
option is not provided, the key will not exist.

`settings.get()` usually works because `None` is false-y, but has the
potential to fail unexpectedly

* fix reference to undefined method

* Avoid use of .get() when existence is immediately assumed

Clean up some uses of get where the code either immediately assumes that
the result is not None or else provides a replacement value, e.g.
through a ternary expression.

Keeping this clean makes error messages more debuggable when unexpected
errors aren't caught.

* Fix API consistency issues

Many sub-classes modify their parents' methods and alter the allowed
arguments. This is a problem because if a function call is written on
the API of the parent class, it will result in an error if it uses
keyword arguments unavailable in the child class, and may fail or yield
unexpected results if it uses positional arguments in a different order.

Even when this can be guaranteed not to be the case through analysis of
the code, it's still a bad practice that can break newly added code
unexpectedly. I've fixed all of these issues that I could find.

* Add considerably more error checking

This is mainly just catching and dealing with type mismatch errors
reported by Pyright. This catches a lot of cases where HTML parsing can
fall through, but not all of them, e.g. IndexError and KeyError are not
type errors. (But failing to prove an optional type has a value is.)

How you deal with these issues is ultimately a matter of some
preference. In two or three places, I removed a perfectly adequate check
that looked for an AttributeError, since exception catching doesn't
satisfy the type checker. But this commit could be revised to leave
these checks in or even do more of them.

A follow up to this should probably add checks on the result of every
`request.get` and `request.post`, as I only did that here where it was
convenient.

* fix erroneous equality check used in place of an assignment

* Remove some incorrect (and probably dead) code

These lines are an implementation of `guess_date_from_id`, which is
supposed to turn a puzzle ID into a `datetime`, but they can't possibly
have worked because you can't call `datetime.strftime` that way, and in
any case the IDs now appear to be random hashes without date
information.

* handle error a bit more robustly

* remove unused imports

* avoid unnecessary 'import *' behavior

* remove unused variables and dead code

* make all exception catching explicit and refactor

* Refactor imports of downloader plugins

This commit changes how importing the downloaders happens slightly.
The point is to avoid making any assumptions in xword_dl.py about
what plugins are available and what methods they have.

This has a number of advantages, for example, that it's trivial for
someone to add a downloader plugin that requires authentication
without adding another special case to the main control flow. This
also happens to make code checkers happy as a side effect, because
they can't determine whether the plugin classes are in the downloader
namespace or not without executing the downloader __init__ code.

The work in this commit is prefatory to an additional refactor which
would create a generic Downloader class that automatically finds
plugins and abstracts the control flow away from the application logic
in xword_dl.py.

* Make import of downloader modules much cleaner

Gets rid of a lot of the complicated introspection required to pick
up the available downloader classes, and also moves all of this
logic out of xword_dl.py.

This change could potentially be amended to add the plugins to the
downloader namespace as before, but after this change along with the
last several, this has no real benefit and leaves the namespace
cluttered.

* remove unnecessary import in __init__.py

* Simplify overcomplicated use of hasattr

With this commit the entire repository is Pyright clean!!

* fix issue where refactor broke authentication

* Additional typing improvements

Simplifies __get_subclasses(), and adds type annotations to important
methods in __init__.py, xword_dl.py, and utils.py. The result of this
work was to reveal a number of issues with API inconsistency, which
this commit fixes as well.

* Make __get_subclasses generic

More appropriate as this function isn't specific to the downloader
types.

* type checking: use Python 3.11 compatible generic signature

* fix typo

* check for non-empty commands in help text

* correctly filter out downloaders that have no independent matches_url implementation

* ruff format

* update min version to 3.10 because of typing union syntax

* plugin feature detection: clean up method override checks

---------

Co-authored-by: Parker Higgins <parker@parkerhiggins.net>
Most of the downloaders follow a similar pattern of creating a
puzzle file with `puz.Puzzle()` in their `parse_xword` method
and returning this puzzle file to the controller. This refactor
changes this to make puzzle files attributes of the
`BaseDownloader` class, which the downloaders then use as needed.

This has several benefits:

 * The `Puzzle` interface is now abstract, from the point of view
   of the downloaders. None of them imports `puz` any more. This
   enables a potential future PR to allow swapping out `puz` for
   a compatible implementation, e.g. one that would create files
   in a different format like .ipuz (libipuz).

 * As part of this PR, we add a flag `-1` `--v1` that creates PUZ
   v1.4 files, and store files as UTF-8 by default, which requires
   PUZ v2.0. Not all software supports these files, so we continue
   to support the legacy format. For software that does (e.g.
   Gnome Crosswords), we don't have to replace emoji characters,
   which are becoming increasingly common in digital crosswords.
@lmfont
lmfont force-pushed the refactor-puz-use-and-support-v2 branch from ac792e8 to a147048 Compare May 31, 2025 03:53
@lmfont

lmfont commented May 31, 2025

Copy link
Copy Markdown
Contributor Author

Okay, I've picked my changes on top of main and force pushed, so this should be ready to go if you're interested in it @thisisparker. This is huge for me in terms of getting working emoij out of the box in solvers like Gnome Crosswords.

I acknowledge there might be concerns about how other solvers would respond to making PUZv2 the default. For all I know, there are breaking changes in PUZv2 that some solvers haven't adapted to. However, I don't think we need to worry about that in this case, because the puzpy library appears to use no features of PUZv2 other than saving the files as UTF-8 and changing the PUZ version number in the file. The only way this could break a solver is if one actively refuses to open v2 files, or can't handle UTF-8, and both seem pretty unlikely.

Still, it's worth testing with whatever solver you use. If necessary, this can be changed to make v1 the default.

Edit: there might still be issues with rebus table encoding? I haven't looked into this since the comment above.

@lmfont

lmfont commented Sep 18, 2025

Copy link
Copy Markdown
Contributor Author

I'm going to try to resolve the conflicts on this branch and hopefully we can merge this. Note to self: check if a new version of puzpy has been released, because there's now a method to set the puzzle version. alexdej/puzpy@0c85bae

@thisisparker

Copy link
Copy Markdown
Owner

Amazing. Let me know if you want me to take a crack at resolving the conflicts (but I think you may be better situated). I just got a notification on another puzpy issue that a new release is forthcoming, which is exciting!

@thisisparker thisisparker mentioned this pull request Sep 28, 2025
@lmfont
lmfont force-pushed the refactor-puz-use-and-support-v2 branch from a147048 to e87c489 Compare July 20, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants