Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Chsudeepta
force-pushed
the
Ticket3396_ShowGlobalMacros
branch
from
e0332e2 to
06ce22e
Compare
|
|
||
| """Contains all the code for defining a configuration or component""" | ||
|
|
||
| # ruff: noqa: I001 |
There was a problem hiding this comment.
must: sort imports and remove noqa
| def __init__( | ||
| self, | ||
| name: str, | ||
| macros: Dict[str, str], |
There was a problem hiding this comment.
nit:
| macros: Dict[str, str], | |
| macros: dict[str, str], |
| self.macros = macros | ||
|
|
||
| @staticmethod | ||
| def _dict_to_list(in_dict: Dict[str, Any]) -> List[Any]: |
There was a problem hiding this comment.
nit:
| def _dict_to_list(in_dict: Dict[str, Any]) -> List[Any]: | |
| def _dict_to_list(in_dict: dict[str, Any]) -> list[Any]: |
| """ | ||
| self.name = name | ||
|
|
||
| if macros is None: |
There was a problem hiding this comment.
should: macros not type-hinted as being able to be set to None.
| def get(self, name: str) -> None: | ||
| return self.__getattribute__(name) | ||
|
|
||
| def __getitem__(self, name: str) -> None: | ||
| return self.__getattribute__(name) |
There was a problem hiding this comment.
must: what are these doing?
| return self.__getattribute__(name) | ||
|
|
||
|
|
||
| class GlobalmacroHelper: |
There was a problem hiding this comment.
nit: does it need to be a class with one method at all - can it just be a free function?
| # along with this program; if not, you can obtain a copy from | ||
| # https://www.eclipse.org/org/documents/epl-v10.php or | ||
| # http://opensource.org/licenses/eclipse-1.0.php | ||
| # ruff: noqa: I001 |
| """ | ||
|
|
||
| @staticmethod | ||
| def row_to_globalmacro(globalmacros: Dict, row: str) -> None: |
There was a problem hiding this comment.
should: This function mutates its input argument which is more confusing than necessary.
I feel this function could return tuple[str, str] | None instead, and then the caller adds it to a collection if necessary.
| configuration.components = components | ||
| configuration.meta = meta | ||
| for key, value in globalmacros.items(): | ||
| configuration.add_globalmacro(key, value) |
There was a problem hiding this comment.
should: this method is already very long and has many responsibilities.
I would like to move as much of the globals.txt parsing logic out of this method as possible - conceptually I would like just one line to remain in this function:
configuration.globalmacros = parse_globals()This will also make the parse_globals function much easier to unit-test.
| if ioc_separator in ioc_macro: | ||
| ioc, macro = ioc_macro.split(ioc_separator, maxsplit=1) | ||
| else: | ||
| ioc = all_iocs |
There was a problem hiding this comment.
should: We seem to have a "special" key of __ here meaning "all"? If so that's quite confusing.
2 participants
Description of work
This is the blockserver change to add the globals.txt settings to the config.
To test
This address the issue described here
Acceptance criteria
The globals.txt settings should get added to the current config and passed on to the GUI.
Code Review
Functional Tests
Final steps