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

refactor: Drop dependency on fact_helper_file #1140

Merged
merged 2 commits into from
Sep 12, 2024
Merged

refactor: Drop dependency on fact_helper_file #1140

merged 2 commits into from
Sep 12, 2024

Conversation

maringuu
Copy link
Collaborator

@maringuu maringuu commented Oct 10, 2023 •
edited
Loading

This uses https://github.com/maringuu/fact-mime-database which is imo the only relevant thing that fact_helper_file provides.

Before merging we should somehow incoperate that fact_helper_file/mime/custom_mime_internal is missing.
If would be great if we could add this somehow add to the FACT_core repo to make fact-mime-database not use its generality.

@maringuu maringuu force-pushed the drop-fact-helper-file branch from 402d528 to ea0845f Compare October 10, 2023 09:47
@maringuu
Copy link
Collaborator Author

While I don't really care note that the repo is in my namespace and not fkiecad's.

@maringuu maringuu marked this pull request as ready for review October 10, 2023 09:48
@maringuu maringuu requested a review from jstucke October 10, 2023 09:48
@maringuu maringuu marked this pull request as draft October 10, 2023 11:15
@maringuu maringuu force-pushed the drop-fact-helper-file branch from ea0845f to 855101f Compare October 12, 2023 10:30
@maringuu
Copy link
Collaborator Author

maringuu commented Oct 12, 2023 •
edited
Loading

This is blocked on ahupp/python-magic#304 which allows us to delete helperFunctions/magic.py completely.
Edit: To actually get rid of it we also need to solve ahupp/python-magic#305

@maringuu maringuu mentioned this pull request Oct 12, 2023
Closed
@maringuu maringuu force-pushed the drop-fact-helper-file branch from 855101f to fda3e23 Compare October 18, 2023 15:15
@maringuu
Copy link
Collaborator Author

Seems that I and the python-magic developer disagree on what a good public API for the magic module would be.
I updated our magic wrapper to be what I wanted in pymagic's API.

Regarding the FACT extractor I don't think we need to copy this code since the only thing it ever needs is Magic.from_file(path, mime=true).
So it could just provide a global instance of magic.Magic and everything works as expected. If we even used the MAGIC environment variable to specify where the additonal magic database is, we can just use magic.from_file instead of creating an instance of Magic.

@maringuu maringuu force-pushed the drop-fact-helper-file branch from fda3e23 to 02405dd Compare October 18, 2023 15:24
@maringuu maringuu mentioned this pull request Nov 7, 2023
Closed
@maringuu maringuu force-pushed the drop-fact-helper-file branch from 02405dd to 3722e15 Compare November 7, 2023 09:05
@maringuu
Copy link
Collaborator Author

maringuu commented Nov 7, 2023

Assuming that fkie-cad/fact_extractor#128 works and is merged this is ready for review.

@maringuu maringuu marked this pull request as ready for review November 7, 2023 09:06
@maringuu maringuu force-pushed the drop-fact-helper-file branch 3 times, most recently from c5b6ce0 to 236a975 Compare November 14, 2023 07:37
@maringuu maringuu force-pushed the drop-fact-helper-file branch from 236a975 to eba377d Compare March 6, 2024 12:43
@maringuu maringuu force-pushed the drop-fact-helper-file branch from eba377d to 97bede4 Compare May 13, 2024 13:41
@maringuu maringuu force-pushed the drop-fact-helper-file branch from 97bede4 to ebf366f Compare June 14, 2024 08:02
@maringuu maringuu force-pushed the drop-fact-helper-file branch from ebf366f to ab9cb48 Compare August 1, 2024 12:21
@maringuu
Copy link
Collaborator Author

maringuu commented Aug 1, 2024 •
edited
Loading

@jstucke Assuming the CI passes, this should be mergeable as is.

Assuming that fkie-cad/fact_extractor#128 works and is merged this is ready for review.

Note that this is not true. Why shouldn't we drop a dependency when we can?!

@maringuu maringuu force-pushed the drop-fact-helper-file branch from ab9cb48 to e2022de Compare August 15, 2024 15:11
jstucke
jstucke requested changes Aug 21, 2024
View reviewed changes
src/install/common.py Outdated Show resolved Hide resolved
src/helperFunctions/magic.py Show resolved Hide resolved
src/helperFunctions/magic.py Outdated Show resolved Hide resolved
src/helperFunctions/magic.py Outdated Show resolved Hide resolved
src/helperFunctions/magic.py Outdated Show resolved Hide resolved
src/install/internal_symlink_magic Outdated Show resolved Hide resolved
src/install/common.py Outdated Show resolved Hide resolved
@maringuu maringuu force-pushed the drop-fact-helper-file branch from e2022de to 4cb3ea5 Compare August 22, 2024 13:04
@maringuu maringuu force-pushed the drop-fact-helper-file branch 2 times, most recently from 0eb2e2c to 6073769 Compare September 12, 2024 09:07
@maringuu maringuu force-pushed the drop-fact-helper-file branch 2 times, most recently from dbccbbe to 7f7fc49 Compare September 12, 2024 10:06
jstucke
jstucke reviewed Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@jstucke jstucke left a comment

Choose a reason for hiding this comment

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

Everything looks good except for a small typo

src/helperFunctions/magic.py Outdated Show resolved Hide resolved
src/helperFunctions/magic.py Show resolved Hide resolved
@maringuu
refactor: Drop fact_helper_file dependency
67e75b2 
It is replaced by the much simpler magic.py.
Note that we do not compile the custom magic files, as it would need
to be updated once `file` is updated.
@maringuu maringuu force-pushed the drop-fact-helper-file branch from 7f7fc49 to 67e75b2 Compare September 12, 2024 13:20
jstucke
jstucke approved these changes Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@jstucke jstucke left a comment

Choose a reason for hiding this comment

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

LGTM!

@maringuu maringuu merged commit ceb69ec into master Sep 12, 2024
10 checks passed
@maringuu maringuu deleted the drop-fact-helper-file branch September 12, 2024 13:33
@maringuu maringuu mentioned this pull request Sep 19, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstucke jstucke jstucke approved these changes

Assignees

@maringuu maringuu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maringuu @jstucke