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.

Sign up for GitHub

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

Jump to bottom

Add --copy-pages option to copy pages.jsonl/extraPages.jsonl directly to WACZ #43

Merged
merged 12 commits into from
Apr 11, 2024
Merged

Add --copy-pages option to copy pages.jsonl/extraPages.jsonl directly to WACZ #43

merged 12 commits into from
Apr 11, 2024

Conversation

tw4l
Copy link
Member

@tw4l tw4l commented Apr 11, 2024 •
edited
Loading

Fixes #42

Also bumps version to 0.5.0

@tw4l tw4l requested a review from ikreymer April 11, 2024 19:27
@ikreymer ikreymer mentioned this pull request Apr 11, 2024
Merged
ikreymer
ikreymer reviewed Apr 11, 2024
View reviewed changes
ikreymer
ikreymer reviewed Apr 11, 2024
View reviewed changes
ikreymer
ikreymer reviewed Apr 11, 2024
View reviewed changes
ikreymer
ikreymer reviewed Apr 11, 2024
View reviewed changes
@ikreymer
Copy link
Member

Hm, I do wonder if we should switch the logic, and make this be the default, while previous behavior should be something like --validate-page-records? Since we are still parsing and validating each line, but not attempting to validate with each WARC record

@tw4l
Copy link
Member Author

tw4l commented Apr 11, 2024

Hm, I do wonder if we should switch the logic, and make this be the default, while previous behavior should be something like --validate-page-records? Since we are still parsing and validating each line, but not attempting to validate with each WARC record

I think it might be safer from a breaking changes perspective to keep it as-is? I see your point though, if you have a strong preference can make the change.

@ikreymer
Copy link
Member

Hm, I do wonder if we should switch the logic, and make this be the default, while previous behavior should be something like --validate-page-records? Since we are still parsing and validating each line, but not attempting to validate with each WARC record

I think it might be safer from a breaking changes perspective to keep it as-is? I see your point though, if you have a strong preference can make the change.

Nope, no strong preference, can keep as is!

ikreymer
ikreymer reviewed Apr 11, 2024
View reviewed changes
ikreymer
ikreymer approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@ikreymer ikreymer left a comment

Choose a reason for hiding this comment

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

Looks good! Crawler tests pass with this branch!

@tw4l tw4l merged commit 1e8f724 into main Apr 11, 2024
6 checks passed
@tw4l tw4l deleted the copy-pages-files branch April 11, 2024 20:13
ikreymer added a commit to webrecorder/browsertrix-crawler that referenced this pull request Apr 11, 2024
@ikreymer
Separate writing pages to pages.jsonl + extraPages.jsonl to use with …
b5f3238 
…new py-wacz (#535)

Cherry-picked from the use-js-wacz branch, now implementing separate
writing of pages.jsonl / extraPages.jsonl to be used with py-wacz and
new `--copy-page-files` flag.

Dependent on py-wacz 0.5.0 (via webrecorder/py-wacz#43)

---------
Co-authored-by: Tessa Walsh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikreymer ikreymer ikreymer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add --copy-pages option to copy pages.jsonl/extraPages.jsonl as-is into WACZ
2 participants
@tw4l @ikreymer