Skip to content
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

Re-adding caching of Metas & fixing how files are scanned and ref dependencies #91

Merged
merged 22 commits into from
Feb 2, 2019

Conversation

weaverryan
Copy link
Collaborator

Hi!

I'm trying to re-add the caching of the metas files. It's fairly trivial, except that there are/were two problems:

  1. In Scanner, the $rst variable is always missing its .rst extension. And so, the file_exists($rst) call always returned false.

  2. Fixing that meant that a whole sub-system of "loading from an existing meta instead of re-parsing" was activated, and it's acting strangely:

filectime(): stat failed for /Users/weaverryan/Sites/os/rst-parser/tests/Builder/input//subdir/test-anchor.rst

You can see this in the new failed test: it's trying to get the filectime of some test-anchor.rst file. That is not a file, but actually an inline anchor from one of the source files (tests/Builder/input/subdir/index.rst):

This is a :ref:`test anchor <test-anchor>`

...

.. _test-anchor:

...

For some reason, this test-anchor is being seen as a dependency of the MetaEntry for this subdir/index.rst file. I don't know much about how the anchors are resolved and how this relates to dependencies, so I would love any guidance you may have.

Thanks!

@jwage
Copy link
Member

jwage commented Jan 11, 2019

Currently I don't know. I remember seeing this weirdness where some things were seen as dependencies when they should not be and I have not tracked that down yet.

@weaverryan
Copy link
Collaborator Author

I'm still checking into this - and have identified several related items.

@weaverryan weaverryan force-pushed the re-add-meta-caching branch 2 times, most recently from ac3905a to c8b06a6 Compare January 18, 2019 19:32
@weaverryan weaverryan changed the title [WIP] Re-adding caching of Metas, but shows dependencies problem [WIP] Re-adding caching of Metas & fixing how files are scanned and ref dependencies Jan 18, 2019
@weaverryan
Copy link
Collaborator Author

This is still WIP, but it's close and I'm now fixing a bunch of things.

Most important, the way that we were scanning for files was wrong and WAY too complex. I checked Sphinx: if i have a file foo.rst that is not referenced by ANY other files in ANY ways, it IS still rendered into HTML (but I get a warning that it's an "orphan" - not in any toctree). Our logic was super complex - we were following dependencies (toctree, doc & ref) in one file to find the next files.

I've now fixed and simplified that: Scanner simply finds all .rst files in a directory. Done. This made it much easier to figure out if a file needs to be parsed, or if its MetaEntry (and dependencies) are still fresh so it doesn't need to be.

@weaverryan weaverryan changed the title [WIP] Re-adding caching of Metas & fixing how files are scanned and ref dependencies Re-adding caching of Metas & fixing how files are scanned and ref dependencies Feb 1, 2019
It's much simpler: Sphinx finds all files in the given directory,
recursively, that have the .rst file extension. And it parses all
of them. There is no need to follow "dependencies" to find more things
to parse: just parse all files you find. Sure, then later you might
be able to give a warning if some documents you rendered were not
included in any toctree's - but you will still render those orphan
documents.
The problem is that a ref called "some-link" is not a document
filename, but it is/was being added into dependencies. The solution
is to track which dependencies are unresolved, and resolve them
later once we can.
Before this commit, if there were 2 :ref: to the same spot, because
"references" were not stored uniquely on Environment, we only resolved
the first, not the second. But, if we simply stored $this->dependencies
as a unique array on Environment, we could have a collision between
a :doc: and a :ref: with the same name. The hack of prefixing the
unresolved dependencies was added to work around this.
For example, if we had :ref:`foo` in subdir/index, then
it would be changed to `subdir/foo` and we wouldn't be
able to resolve the `foo` reference later.
lib/Meta/MetaEntry.php Outdated Show resolved Hide resolved
lib/Builder.php Outdated Show resolved Hide resolved
lib/Builder.php Outdated Show resolved Hide resolved
@weaverryan
Copy link
Collaborator Author

Feedback handled! :)

@jwage jwage added the Enhancement New feature or request label Feb 2, 2019
@jwage jwage self-assigned this Feb 2, 2019
@jwage jwage added this to the 0.0.2 milestone Feb 2, 2019
Copy link
Member

@jwage jwage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jwage jwage merged commit e94cf58 into doctrine:master Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants