Port #237 to rhel8 branch #243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Separates code for determining target file name from code that fetches the file from URI.
We will not split URI into 2 parts and then pass it as arguments to methods. Instead we will keep the whole URI as a property of ContentBringer instances. This change will simplify the code.
The argument of various methods in this class that holds the destination directory name is always set to self.CONTENT_DOWNLOAD_LOCATION, which means that these methods don't need to have this argument but they can use self.CONTENT_DOWNLOAD_LOCATION directly.
The static method ContentBringer.__get_dest_file_name is called only in a single place with arguments self.content_uri and self.CONTENT_DOWNLOAD_LOCATION which are both members of the object. So it can become also a property of the object.
The destination file name depends on URI and changes only if the URI is set. It doesn't make sense to compute the destination file name each time it's requested. Instead, we can compute it only once, when the URI is changed.
The parameter report_callback isn't used anywhere.
Split _finish_actual_fetch into 2 methods: 1. _finish_actual_fetch 2. _analyze_fetched_content The reason is to separate fetching from content analysis.
We know the path where the file is downloaded, we have the path stored in self.dest_file_name, that means we don't need to pass the path as an argument
The self.fetched_content isn't used anywhere.
Pass the target URL as a parameter to fetch_content instead of digging it from the PolicyData Class. This change will simplify us to remove PolicyData class from ContentBringer in future.
Move method use_system_content from ContentBringer class to PolicyData class. The reason is that this method only manipulates the PolicyData instance referenced from the ContentBringer so it should be a responsibility of the PolicyData instance.
The outcome of this move is that self._addon_data won't be directly read in the get_preferred_tailoring() method, and that will allow further refactoring in future.
The preferred tailoring file is a property of the ObtainedContent class, so the get_preffered_tailoring method should belong to the ObtainedContent
The outcome of this move is that self._addon_data won't be directly read in the get_preferred_content() method, and that will allow further refactoring in future.
Move method get_preffered_content to the ObtainedContent class because the preffered content path is a property of the ObtainedContent class.
Move the use_downloaded_content method from the ContentBringer to the PolicyData class. This method mostly manipulates the PolicyData class members, so the PolicyData is a more suitable place for the method.
This change will cause that allow_one_expected_tailoring_or_no_tailoring will not directl acess self._addon_data, therefore will not depend on self._addon_data and can be usable without self._addon_data.
Prevent shadowing, the name "expected_path" is already used above with different value.
Don't access self._addon_data directly in filter_discovered_content but instead pass them as parameter. This will reduce the dependency on the PolicyData class (self._addon_data).
Stop accessing self._addon_data in the _analyze_fetched_content to reduce dependency on PolicyData
Don't access self._addon_data directly in finish_content_fetch but instead pass them as parameter. This will reduce the dependency on the PolicyData class (self._addon_data).
After recent changes, the PolicyData instance isn't used in the ContentBringer at all.
This method doesn't access self and isn't oused outside of the class.
This method doesn't use self so it can be a static method
Change allow_one_expected_tailoring_or_no_tailoring method to private static method because it doesn't access self and it isn't used outside of this class.
Change filter_discovered_content to private static method. This method doesn't read self and is used only inside this class.
The method gather_available_files accesses only the class members and doesn't access self otherwise and isn't used outside of this class.
Change the _analyze_fetched_content method to private static method because this method doesn't use self (apart form accessing the constant) and is called only inside this class.
Move all static methods from ContentBringer out to a new class ContentAnalyzer. This faciliates the responsiibilities split. ContentBringer contains code related to fetching content from URI resources. ContentAnalyzer contains code analyzing the fetched content.
This commit will move content analysis away from the ContentBringer.finish_content_fetch() method. Therefore, the ContentBringer class will be responsible only for download of the content and not for the analysis of the downloaded content. Instead, there will be a new method static public method content_discovery.ContentAnalyzer.analyze() and we will add calling this method everywhere where ContentBringer.finish_content_fetch() is called.
These parameters of finish_content_fetch aren't used in it.
From now on, we will call finish_content_fetch only if fetch_content has been called. These 2 calls will therefore always form a kind of "pair" of calls.
The function update_progress_label isn't used anywhere.
Move the decision whether to verify fingerprint outside from the `finish_content_fetch` method. The fingerprint will be verified if the `fingerprint` parameter of the method is set (is not None). This method is called from 2 places. When calling from _init_data_fetch, the dest_filename is always not None. And, when calling from `PrepareValidContent.run()` we can set fingerprint to a non-None value if the content_dest is set. The reason for this change is that the `finish_content_fetch` method will not depend on the destination content file name so it will reduce amount of needed parameters.
We will pass the failure callback parameter in the constructor instead of the individual methods, because it's always the same function, it doesn't differ in different methods and depends only on the caller class.
Even if this breaks othe object-oriented principles, the PolicyData should remain a data structure, an interface that other classes understand and use. This data structure shouldn't know that a thing s.a. "system content" or "downloadable content" exist.
|
The inspection completed: 12 updated code elements |
matejak
approved these changes
Apr 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a port of #237 to rhel8 branch.