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

Rom file and hashing refactor #1369

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Conversation

gantoine
Copy link
Member

@gantoine gantoine commented Dec 21, 2024

Alright this bad boy is ready for review! The gist of changes can be found in the migration file, but basically this PR:

  • Creates a model for RomFile and migrates rom.files over to it
  • Renames file_* columns on Rom to fs_*, to clarify that these are filesystem properties
  • Runs hash calculations in a background task separate from the scan
  • Deeply scans game folders for content like DLC, patches, etc.

I've also cleaned up a lot of pylint errors, added extra checks all over the place for bad data/states, and added HASH_SCAN as a scan type in the UI.

@gantoine gantoine changed the base branch from release to master December 21, 2024 00:14
Copy link

github-actions bot commented Dec 21, 2024

Test Results

93 tests  ±0   93 ✅ ±0   26s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 65ee2ce. ± Comparison against base commit 07c7883.

♻️ This comment has been updated with latest results.

@gantoine gantoine force-pushed the rom-hashing-background-task branch from fb79d55 to bb7913e Compare December 22, 2024 18:55
@gantoine gantoine self-assigned this Dec 30, 2024
"xbox-360",
"xboxone",
)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

moved the location of this list and populated it with more platforms

@gantoine gantoine changed the title Rom file hashing refactor Rom file and hashing refactor Jan 16, 2025
@gantoine gantoine requested review from zurdi15 and adamantike and removed request for zurdi15 January 16, 2025 17:38
@gantoine gantoine marked this pull request as ready for review January 16, 2025 17:38
backend/endpoints/firmware.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/endpoints/rom.py Outdated Show resolved Hide resolved
backend/handler/database/roms_handler.py Outdated Show resolved Hide resolved
Comment on lines +252 to +254
category
for category in RomFileCategory
if category_matches(category.value, path_parts_lower)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a future improvement, I think the way a category matches should be configurable by the user. They could provide a mapping in case they prefer to use a different term for each category.

First example that comes to mind is a different language: patch: "parche"

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure something to look into, but if we're already being strict about the filesystem structure there's no reason we can't mandate specific names for DLC/folders/etc.

Comment on lines +63 to +64
def full_path(self) -> str:
return f"{self.file_path}/{self.file_name}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This could return a Path, and use a notation like Path(self.file_path) / self.file_name

@gantoine gantoine requested a review from adamantike January 18, 2025 01:07
Copy link
Collaborator

@adamantike adamantike left a comment

Choose a reason for hiding this comment

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

Amazing work, looking forward to everything this change unblocks! 🚀

@gantoine
Copy link
Member Author

Thanks for the review! I think we should release a patch version in a couple days with the fixed we have in the pipeline, then I'll merge this in. That way this won't block anything if something major is found.

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.

2 participants