Skip to content

Conversation

@sdn4z
Copy link
Collaborator

@sdn4z sdn4z commented Nov 8, 2024

First MR towards having cache file, that will save the trusted packages in the local file system (#143).

Created a new class FileHandler, which will hold all the interactions towards reading/writing files.

Adapted AbstractParser to use it.

In the following MRs:

  • adapt ConfigHandler to also use it.
  • save trusted packages to a file.

@sdn4z sdn4z changed the title File handler draft chore: Introduce FileHandler Nov 8, 2024
@sdn4z sdn4z force-pushed the file-handler branch 5 times, most recently from d1690f7 to 21d0a65 Compare November 8, 2024 14:14
@sdn4z sdn4z changed the title draft chore: Introduce FileHandler chore: Introduce FileHandler Nov 8, 2024
@sdn4z sdn4z changed the title chore: Introduce FileHandler chore(#143): Introduce FileHandler Nov 8, 2024
@sdn4z sdn4z changed the title chore(#143): Introduce FileHandler chore(elementsinteractive#143): Introduce FileHandler Nov 8, 2024
@sdn4z sdn4z changed the title chore(elementsinteractive#143): Introduce FileHandler chore: Introduce FileHandler Nov 8, 2024
@scastlara scastlara self-requested a review November 8, 2024 14:22
Copy link
Collaborator

@scastlara scastlara left a comment

Choose a reason for hiding this comment

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

Looks reasonable! Can you give me 2cents on what you mean by caching the file? I assume you mean vendoring it in the repository?

Provides basic methods to deal with the dependecies file.
"""

def __init__(self, file_path: str, file_handler: BaseFileHandler = FileHandlerPathlib) -> None:
Copy link
Collaborator

@scastlara scastlara Nov 8, 2024

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, file_path: str, file_handler: BaseFileHandler = FileHandlerPathlib) -> None:
def __init__(self, file_path: str, file_handler: type[BaseFileHandler] = FileHandlerPathlib) -> None:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch!

I was in doubt about receiving the FileHandler object directly or just the type though. For now I think it's fine, as long as we don't have another FileHandler type that requires some other parameters to initialize

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually, I'd say it's best to pass instances instead of classes. Unless you have a very good reason to defer the class instantiation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After checking, I think it's best to just not make AbstractParser receive any FileHandler and assume it's going to use FileHandlerPathlib.

If we wanted to make it receive an object, DependencySelector should tell the AbstractParser which FileHandler to use, but right now I think it's safe to say that we only want one way of opening/reading files. I'd leave it like this, tying AbstractParser to FileHandlerPathlib, and see how it evolves if we agree :)

@sdn4z
Copy link
Collaborator Author

sdn4z commented Nov 8, 2024

Looks reasonable! Can you give me 2cents on what you mean by caching the file? I assume you mean vendoring it in the repository?

That was the original idea but in a way, vendoring it in the repo would be just another source. So twyn would be mirroring the actual TopPypiReferences to guarantee more availability, and then we can provide different options to chose from where to retrieve the data.

The caching I'm referring to in this MR is a local one, so that you would save locally the list of trusted packages without having to download the whole list again.

@sdn4z sdn4z force-pushed the file-handler branch 2 times, most recently from 37518ab to 6e31c9a Compare November 8, 2024 15:45
@sdn4z sdn4z merged commit 959e71f into elementsinteractive:main Nov 8, 2024
@sdn4z sdn4z deleted the file-handler branch November 8, 2024 16:30
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
sdn4z pushed a commit to sdn4z/twyn that referenced this pull request Nov 22, 2024
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