Bug 1968280 - Draft: Landoscript should fetch file modes for newly added or files being removed. #1292
Conversation
…ded or files being removed.
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
This is a fine start. Please take a closer read of https://bugzilla.mozilla.org/show_bug.cgi?id=1968280#c0 to make sure you're making all of those changes, most of which I've highlighted in the comments here.
src/scriptworker_client/github_client.py:176: error: Incompatible types in assignment (expression has type "None", target has type "Union[str, dict[str, Optional[str]]]") [assignment]
This will probably be fixed as a side effect of my requested changes to the get_files signature.
| orig_contents = "" | ||
| fromfile = "/dev/null" | ||
| add_remove_metadata = "new file mode 100644\n" | ||
| add_remove_metadata = f"new file mode {mode or 100644}\n" |
There was a problem hiding this comment.
Rather than having a default here, this function should raise an error if mode is not set when a file is being newly added or removed.
|
|
||
| async def get_files(self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200) -> Dict[str, Union[str, None]]: | ||
| async def get_files( | ||
| self, files: Union[str, List[str]], branch: Optional[str] = None, files_per_request: int = 200, mode: Optional[str] = None |
There was a problem hiding this comment.
This is not quite right - we want a boolean argument called something like fetch_mode that defaults to False. When True it should:
- Change the query to fetch file modes
- Return file modes in the return value
| if mode: | ||
| ret[name] = {"mode": v.get("mode"), "text": v.get("text")} | ||
| else: | ||
| ret[name] = v.get("text") |
There was a problem hiding this comment.
Let's update the value for each file to always be a dict; that will make things easier for callers, and make the function more comprehensible (especially the type signatures).
| toml_files = [info.toml_path for info in android_l10n_import_info.toml_info] | ||
| # we always take the tip of the default branch when importing new strings | ||
| toml_contents = await l10n_github_client.get_files(toml_files) | ||
| toml_contents = await l10n_github_client.get_files(toml_files, mode=None) |
There was a problem hiding this comment.
After you've updated get_files as noted below, the l10n_github_client.get_files calls in this function and android_l10n_sync.py will need to be updated to set fetch_mode=True.
You will also need changes to actually pass the modes along to diff_contents for each file.
| try: | ||
| log.info(f"fetching bump files from github: {files}") | ||
| orig_files = await github_client.get_files(files, branch) | ||
| orig_files = await github_client.get_files(files, branch, mode=None) |
There was a problem hiding this comment.
This specific change should not be needed (see my comment elsewhere about the interface of get_files. You will need to changes to this and all other get_files calls to deal with the fact that it will now be returning dicts for each file, rather than strings.
2 participants
Hi, I'm getting the below error:
src/scriptworker_client/github_client.py:176: error: Incompatible types in assignment (expression has type "None", target has type "Union[str, dict[str, Optional[str]]]") [assignment]I think its happening when I set
ret[name] = None. I tried to change the return type to includeNone, but that ended up creating errors in other places that use this function indirectly. So, can you please let me know how to resolve this or if I need to do any other changes.