Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions fit_file_faker/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from pathlib import Path
from tempfile import NamedTemporaryFile
from typing import Optional, cast
from hashlib import sha256

import questionary
import semver
Expand Down Expand Up @@ -232,12 +233,19 @@ def on_modified(self, event: FileModifiedEvent) -> None:
with uploaded_list.open("r") as f:
uploaded_files = json.load(f)

filename = source_file.name
if filename not in uploaded_files:
uploaded_files.append(filename)
file = {
"name": source_file.name,
"hash": sha256(
open(dir.joinpath(source_file.name), "rb").read()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should use Pathlib.Path and a context manager to ensure the file is properly closed. Also, I'm assuming this was copied from the change in upload_all() without testing? dir doesn't exist in this scope, and will be the built-in dir() function, so this will raise a NameError.

).hexdigest(),
}
if file not in uploaded_files:
uploaded_files.append(file)
with uploaded_list.open("w") as f:
json.dump(uploaded_files, f, indent=2)
_logger.debug(f'Added "{filename}" to uploaded files list')
_logger.debug(
f'Added "{file["name"]}" with hash "{file["hash"]}" to uploaded files list'
)
else:
_logger.warning(
"Found modified file, but not processing because dryrun was requested"
Expand Down Expand Up @@ -384,6 +392,11 @@ def upload_all(
files = [i.replace(str(dir), "").strip("/").strip("\\") for i in files]
# remove files matching what we may have already processed
files = [i for i in files if not i.endswith("_modified.fit")]
# append file hash for each file
files = [
{"name": i, "hash": sha256(open(dir.joinpath(i), "rb").read()).hexdigest()}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should use pathlib.Path and a context manager to ensure the file is properly closed. Something like:

with open(dir.joinpath(source_file.name), "rb") as fh:
    file_hash = sha256(fh.read()).hexdigest()

for i in files
]
# remove files found in the "already uploaded" list
files = [i for i in files if i not in uploaded_files]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Backward compatibility break

When a user upgrades, their existing .uploaded_files.json will contain plain strings from the current code:

["MyNewActivity-5.6.1.fit", "MyNewActivity-5.7.0.fit"]

The new code in this PR converts files to a list of dicts, then runs:

files = [i for i in files if i not in uploaded_files]

A dict {"name": "MyNewActivity-5.6.1.fit", "hash": "..."} will never equal the string "MyNewActivity-5.6.1.fit", so all previously-uploaded files will be re-uploaded on the first run after upgrading.

A migration step is needed after loading uploaded_files:

# Migrate legacy string entries (pre-hash format)
uploaded_files = [
    e if isinstance(e, dict) else {"name": e, "hash": None}
    for e in uploaded_files
]

Then adjust deduplication to match on name-only for legacy entries where hash is None:

def _already_uploaded(f: dict, uploaded: list) -> bool:
    for entry in uploaded:
        if entry["name"] == f["name"]:
            if entry["hash"] is None or entry["hash"] == f["hash"]:
                return True
    return False

files = [f for f in files if not _already_uploaded(f, uploaded_files)]

The same migration is needed in on_modified, which loads the same file independently.


Expand All @@ -394,18 +407,25 @@ def upload_all(
return

for f in files:
_logger.info(f'Processing "{f}"') # type: ignore
_logger.info(f'Processing "{f["name"]}"') # type: ignore

f_path = dir.joinpath(f["name"])

if not preinitialize:
with NamedTemporaryFile(delete=True, delete_on_close=False) as fp:
fit_editor.set_profile(profile)
output = fit_editor.edit_fit(dir.joinpath(f), output=Path(fp.name))
output = fit_editor.edit_fit(f_path, output=Path(fp.name))
if output:
_logger.info("Uploading modified file to Garmin Connect")
upload(
output, profile=profile, original_path=Path(f), dryrun=dryrun
output,
profile=profile,
original_path=Path(f["name"]),
dryrun=dryrun,
)
_logger.debug(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This was a pre-existing issue, but should be fixed here. The message saysAdding "{f["name"]}" with hash "{f["hash"]}" to "uploaded_files", but this log appears inside if output: (i.e. only when upload succeeds). However, uploaded_files.append(f) at line 413 runs unconditionally (outside that if block), which is consistent with existing behavior. The debug log placement is misleading but not broken. The log should go at line 433 right before uploaded_files.append(f)

f'Adding "{f["name"]}" with hash "{f["hash"]}" to "uploaded_files"'
)
_logger.debug(f'Adding "{f}" to "uploaded_files"')
else:
_logger.info(
"Preinitialize was requested, so just marking as uploaded (not actually processing)"
Expand Down
Loading