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

File tree diff #11646

Draft
wants to merge 4 commits into
base: refactor-search-index-process
Choose a base branch
from
Draft

File tree diff #11646

wants to merge 4 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 3, 2024

This is on top of #11643.

Closes #11319

Ref #11507

Comment on lines 23 to 24
@functools.cache
def _get_page_content(self, page):
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid this may implode our memory usage :D, some options:

  • Just read the content from storage twice.
  • When parsing the content (search indexing), we can calculate the hash of the main content, and save that for later, so we will be saving just the hash, and not the whole content. This will make things faster, but it mixes things...

Copy link
Member Author

Choose a reason for hiding this comment

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

Went with the second idea in ed5377d

Comment on lines +65 to +70
build_id_from_manifest = manifest.get("build", {}).get("id")
if latest_successful_build.id != build_id_from_manifest:
# The manifest is outdated,
# do we want to still use it? do we care?
# Should the caller be responsible to handle this?
return None
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this may depend on how do we want to expose this feature on the addons. I guess maybe the caller or the addons client should be responsible for deciding what to do here, maybe show the ft diff, but with a warning.

@stsewd
Copy link
Member Author

stsewd commented Oct 3, 2024

This is still missing tests and docstrings, but it would be great to have a review if we are good with this direction.

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.

1 participant