This is an archive of the discontinued Mercurial Phabricator instance.

help: create packages for the help text
ClosedPublic

Authored by mharbison72 on Nov 13 2019, 10:02 PM.

Details

Summary

These files need to be loaded as resources with PyOxidizer, instead of using
filesystem representations. AFAICT, the resource loading mechanisms only work
for the named package given to it, and can't reach into a subdirectory.

While here, the help directory is renamed to helptext. Without this, trying
to load external help text crashed in mercurial/help.py when importing .i18n,
saying there's no mercurial.help.i18n module.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Nov 13 2019, 10:02 PM

Looks good to me, but I don't know Python well enough to say if mercurial.help and mercurial.help.internal should be "namespace packages" (which I think is what you're doing with the pkgutil.extend_path() stuff). I'd appreciate it if someone who knows Python better can comment.

We could also consider removing the internals/ subdirectory and replacing it by a internals. prefix to the filename. Just a thought; I don't care much.

Looks good to me, but I don't know Python well enough to say if mercurial.help and mercurial.help.internal should be "namespace packages" (which I think is what you're doing with the pkgutil.extend_path() stuff). I'd appreciate it if someone who knows Python better can comment.

I'm not sure if it's better to make these namespaces or not, but I think it's better to start with regular packages. We can always upgrade them to namespace packages later.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

FWIW I have a series of patches somewhere that teach the help, templates, etc subsystems to use the importlib.resources API if available.

I also have plans to enable PyOxidizer to package resources with more flexibility so we wouldn't necessarily have to rename a bunch of files and add __init__.py files to mark directories as packages so importlib.resources *just works*. I haven't gotten around to it yet. I figured if Mercurial has resources files like this, other projects would, so I would need to teach PyOxidizer to handle wonky layouts like this.

But if we want to continue and support the importlib.resources API so things work with the current state of PyOxidizer, that's fine by me. It just isn't strictly required if there is resistance to mass renaming files and conforming to the wonky requirements imposed today.

FWIW I have a series of patches somewhere that teach the help, templates, etc subsystems to use the importlib.resources API if available.

I'd be happy to see those soon. I had just started working on that.

I also have plans to enable PyOxidizer to package resources with more flexibility so we wouldn't necessarily have to rename a bunch of files and add __init__.py files to mark directories as packages so importlib.resources *just works*. I haven't gotten around to it yet. I figured if Mercurial has resources files like this, other projects would, so I would need to teach PyOxidizer to handle wonky layouts like this.
But if we want to continue and support the importlib.resources API so things work with the current state of PyOxidizer, that's fine by me. It just isn't strictly required if there is resistance to mass renaming files and conforming to the wonky requirements imposed today.

I'm fine with fixing it in mercurial. It seems like the right way of doing it anyway and it's not that much work.

See the series starting at D7415 for the patches I authored a while ago. Please take them over and push them over the finish line if you agree with the approach!