richardmaw-codethink commented on code in PR #2167:
URL: https://github.com/apache/buildstream/pull/2167#discussion_r3758800388
##########
src/buildstream/_frontend/app.py:
##########
@@ -87,9 +93,9 @@ def __init__(self, main_options):
self._detail_profile = Profile(dim=True)
# Cached messages
- self._cached_message_lock = threading.Lock()
- self._cached_message_text = ""
- self._cache_messages = None
+ self._cached_message_lock: Lock = threading.Lock()
Review Comment:
Since Lock has been imported directly I'd be tempted to switch over to using
it.
```suggestion
self._cached_message_lock: Lock = Lock()
```
##########
src/buildstream/_loader/loader.py:
##########
@@ -875,9 +908,10 @@ def provenance_str():
# we haven't yet for this element),
# element._get_cache_key() can fail if used with the
# default _KeyStrength.STRONG.
- basedir = os.path.join(
- self.project.directory, ".bst", "staged-junctions", filename,
element._get_cache_key(_KeyStrength.WEAK)
- )
+ assert self.project.directory, "The loaders project must have a
project directory"
+ key = element._get_cache_key(_KeyStrength.WEAK)
+ assert key, "We expect a weak key is always available"
Review Comment:
When you get to it, it's possible to overload the definition of
_get_cache_key to make the assert unnecessary.
```python3
@overload
def _get_cache_key(self, strength: Literal[_KeyStrength.WEAK]) -> str:
...
@overload
def _get_cache_key(self, strength: Literal[_KeyStrength.STRONG) ->
Optional[str]:
...
def _get_cache_key(self, strength: _KeyStrength = KeyStrength.STRONG) ->
Optional[str]:
if strength == _KeyStrength.STRONG:
return self.__cache_key
else:
return self.__weak_cache_key
```
```suggestion
key = element._get_cache_key(_KeyStrength.WEAK)
```
##########
src/buildstream/_loader/loadcontext.py:
##########
@@ -14,23 +14,31 @@
# Authors:
# Tristan Van Berkom <[email protected]>
+
+from typing import Callable, Optional, TYPE_CHECKING
+
+
from .._exceptions import LoadError
from ..exceptions import LoadErrorReason
from ..types import _ProjectInformation
+if TYPE_CHECKING:
+ from .._context import Context
+ from .._loader.loader import Loader
+
# ProjectLoaders()
#
# An object representing all of the loaders for a given project.
#
class ProjectLoaders:
- def __init__(self, project_name):
+ def __init__(self, project_name: str):
# The project name
self._name = project_name
# A list of all loaded loaders for this project
- self._collect = []
+ self._collect: list["Loader"] = []
Review Comment:
https://docs.python.org/3/library/__future__.html#future__.annotations is
the typical solution recommended for circular definitions
but honestly it's due to become the default in the next release and the
import deprecated to eventually become a syntax error so switching to it is
going to cause more work down the line
##########
src/buildstream/_loader/loader.py:
##########
@@ -841,6 +869,10 @@ def provenance_str():
element = Element._new_from_load_element(load_element)
+ from ..plugins.elements.junction import JunctionElement
Review Comment:
Is this deferred import intentional? It's not going to have the problem of
deferring finding import problems since `Element._new_from_load_element` will
have needed to import it, but it does seem out of place.
##########
src/buildstream/_loader/loader.py:
##########
@@ -622,13 +649,13 @@ def _check_circular_deps(top_element):
# Returns:
# (ScalarNode): The overridding node from this project's junction, or
None
#
- def _search_for_local_override(self, override_path):
+ def _search_for_local_override(self, override_path: str) ->
Optional[ScalarNode]:
junction = self.project.junction
if junction is None:
return None
# Try the override without any link substitutions first
- with suppress(KeyError):
+ if override_path in junction.overrides:
return junction.overrides[override_path]
Review Comment:
I have a style preference for walrusing this kind of thing because otherwise
you hash the key twice, but it's not universally recommended.
```suggestion
if (override_node := junction.overrides.get(override_path)) is not
None:
return override_node
```
##########
src/buildstream/_testing/runcli.py:
##########
@@ -394,11 +433,12 @@ def _invoke(self, cli_object, args=None,
binary_capture=False):
exc_info = sys.exc_info()
finally:
sys.stdout.flush()
+ # sys.stderr.flush()
Review Comment:
I think this is accidentally left over.
##########
src/buildstream/element.py:
##########
@@ -2592,7 +2636,7 @@ def _get_logs(self) -> List[str]:
# (ElementProxy): An ElementProxy to self, for owner.
#
def __get_proxy(self, owner: "Element") -> ElementProxy:
- with suppress(KeyError):
+ if owner in self.__proxies:
return self.__proxies[owner]
Review Comment:
See previous comment about walrus
```suggestion
if (cached_proxy := self.__proxies.get(owner)) is not None:
return cached_proxy
```
##########
src/buildstream/element.py:
##########
@@ -1779,12 +1815,13 @@ def _cache_artifact(self, sandbox, collect):
except DirectoryError:
pass
- # We should always have cache keys already set when caching an artifact
- assert self.__cache_key is not None
- assert self.__artifact._cache_key is not None
+ #
Review Comment:
I suspect this comment is left-over.
```suggestion
```
##########
src/buildstream/_testing/runcli.py:
##########
@@ -333,7 +372,7 @@ def run(self, project=None, silent=False, env=None,
cwd=None, options=None, args
if project:
bst_args += ["--directory", str(project)]
- for option, value in options:
+ for option, value in batched(options, n=2):
Review Comment:
Nice catch!
##########
src/buildstream/_frontend/app.py:
##########
@@ -587,9 +596,9 @@ def _render_status(self):
# Handle ^C SIGINT interruptions in the scheduling main loop
#
def _interrupt_handler(self):
-
+ assert self.stream, "Must have a Stream in App for this"
# Only handle ^C interactively in interactive mode
- if not self.interactive:
+ if not self.interactive: #
Review Comment:
Leftover unfinished comment, presumably replaced by the assertion.
```suggestion
if not self.interactive:
```
##########
src/buildstream/element.py:
##########
@@ -1284,7 +1311,7 @@ def _buildable(self):
#
# None is returned if information for the cache key is missing.
#
- def _get_cache_key(self, strength=_KeyStrength.STRONG):
+ def _get_cache_key(self, strength=_KeyStrength.STRONG) -> str | None:
Review Comment:
See previous comment about _get_cache_key
```suggestion
@overload
def _get_cache_key(self, strength: Literal[_KeyStrength.WEAK]) -> str:
...
@overload
def _get_cache_key(self, strength: Literal[_KeyStrength.STRONG) ->
Optional[str]:
...
def _get_cache_key(self, strength: _KeyStrength = _KeyStrength.STRONG)
-> Optional[str]:
if strength == _KeyStrength.STRONG:
return self.__cache_key
else:
return self.__weak_cache_key
```
##########
src/buildstream/element.py:
##########
@@ -2310,7 +2346,10 @@ def _fetch(self, fetch_original=False):
#
# None is returned if information for the cache key is missing.
#
- def _calculate_cache_key(self, dependencies, weak_cache_key=None):
+ def _calculate_cache_key(self, dependencies: list[list[str]],
weak_cache_key: Optional[str] = None) -> str | None:
+ assert self.__sandbox_config, "Element should have a sadbox config to
calculate cache key"
Review Comment:
```suggestion
assert self.__sandbox_config, "Element should have a sandbox config
to calculate cache key"
```
##########
src/buildstream/element.py:
##########
@@ -1051,25 +1074,26 @@ def _stage_dependency_artifacts(self, sandbox, scope,
*, path=None, include=None
# (Element): A newly created Element instance
#
@classmethod
- def _new_from_load_element(cls, load_element, task=None):
+ def _new_from_load_element(cls, load_element: LoadElement, task:
Optional[Task] = None) -> Element:
if not load_element.first_pass:
load_element.project.ensure_fully_loaded()
- with suppress(KeyError):
+ if load_element in cls.__instantiated_elements:
return cls.__instantiated_elements[load_element]
Review Comment:
See previous comment.
```suggestion
if (instantiated_element :=
cls.__instantiated_elements.get(load_element)) is not None:
return instantiated_element
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
