-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add keep2strava #653
add keep2strava #653
Conversation
some of code are the same as keep_sync.py can we extract them? |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I've made the appropriate changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Let's proceed by making some minor changes.
run_page/keep_sync.py
Outdated
@@ -318,5 +340,13 @@ def run_keep_sync(email, password, with_download_gpx=False): | |||
action="store_true", | |||
help="get all keep data to gpx and download", | |||
) | |||
parser.add_argument( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also add this argument to readme
run_page/keep_to_strava.py
Outdated
""" | ||
|
||
|
||
def get_all_keep_tracks(email, password, old_tracks_ids, with_download_gpx=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part is duplicated
run_page/keep_to_strava.py
Outdated
|
||
|
||
def run_keep_sync(email, password, with_download_gpx=True): | ||
keep2strava_bk_path = os.path.join(OUTPUT_DIR, "keep2strava.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to make the file name and path to a static variable
run_page/keep_sync.py
Outdated
@@ -161,31 +177,35 @@ def parse_raw_data_to_nametuple( | |||
return namedtuple("x", d.keys())(*d.values()) | |||
|
|||
|
|||
def get_all_keep_tracks(email, password, old_tracks_ids, with_download_gpx=False): | |||
def get_all_keep_tracks(email, password, old_tracks_ids, with_download_gpx=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't change the default value here
run_page/keep_to_strava.py
Outdated
json.dump(content, f, indent=0) | ||
|
||
# del gpx | ||
for track in new_tracks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it to delete the uploaded_file_paths
? or we can delete the file while the file is uploaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since GPX files that fail to upload will still be uploaded next time, it might be better to delete them all to keep GPXOUT clean.
Deleting files while uploading, I'm not sure if I'm going to have a problem similar to ”# Fix the issue that the github action runs too fast, resulting in unsuccessful file generation“ in nike_to_strava.py. If I need to add sleep to avoid it, then I might as well delete them last to keep the execution efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding comments would be helpful to clarify the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in the other files do, this project is mainly for running, I think the newly introduced IS_ONLY_RUN = False
can be a breaking change. Others who only need running will have to modify their script files and add --only-run
as well or their projects will sync all sport types(even they don't need), this is a little wired. it's more reasonable in @ben-29 's [workout page] (https://github.com/ben-29/workouts_page) project.
A suggested approach could be by introducing additional argument --sync-all-types
or --sync-types
then followed needed sport types. users can choose their sport types they need, and default is running
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also open PR in Workout Page after this PR has been merged @SongJgit
@@ -18,6 +18,7 @@ on: | |||
- run_page/gpx_sync.py | |||
- run_page/tcx_sync.py | |||
- run_page/garmin_to_strava_sync.py | |||
- run_page/keep_to_strava_sync.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add this job in the below jobs section
README-CN.md
Outdated
|
||
```yaml | ||
- name: Run sync Strava script | ||
if: env.RUN_TYPE == 'strava' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the new added RUN_TYPE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also open PR in Workout Page after this PR has been merged @SongJgit
README-CN.md
Outdated
```yaml | ||
RUN_TYPE: starva | ||
``` | ||
- if: env.RUN_TYPE == 'keep_to_strava'改为 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的两个方案,我认为有点绕,不太好理解和操作。
让用户只配置 RUN_TYPE
就好,再去改下面的job分支,对新手用户不太友好;同时,如果用户已经改了,再想把RUN_TYPE
改回 'keep_to_strava' 时,会难以操作。另外,这么改也对只用 strava 不用 keep 的用户造成困扰。
建议:针对第3点的2种方案,增加两个 RUN_TYPE
,比如分别为 keep_to_starva
和 keep_to_starva_sync
,用户只用在这2个里面选择,无须再更改其他修改。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
.github/workflows/run_data_sync.yml
Outdated
# If you only want to sync `type running` modify args --sync-types running, default script is to sync all data (rides, hikes and runs). | ||
# If you only want to sync data from keep to strava, and the running page display the data from strava, you need to change the "if: env.RUN_TYPE == 'keep_to_strava'" -> "if: env.RUN_TYPE == 'strava'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice comments
README-CN.md
Outdated
```yaml | ||
RUN_TYPE: starva | ||
``` | ||
- if: env.RUN_TYPE == 'keep_to_strava'改为 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
run_page/keep_sync.py
Outdated
return tracks | ||
|
||
|
||
def parse_points_to_gpx(run_points_data, start_time): | ||
def parse_points_to_gpx(run_points_data, start_time, type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not name type here, type is a key word in Python
sport_type or _type
@@ -161,31 +178,34 @@ def parse_raw_data_to_nametuple( | |||
return namedtuple("x", d.keys())(*d.values()) | |||
|
|||
|
|||
def get_all_keep_tracks(email, password, old_tracks_ids, with_download_gpx=False): | |||
def get_all_keep_tracks( | |||
email, password, old_tracks_ids, keep_sports_data_api, with_download_gpx=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keep_sports_data_api is not a good name, it confuse me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it is named this way because the variable is intended to be used to assign a value to RUN_DATA_API. Hence the inevitable ambiguity.
except: | ||
print(f"wrong id {keep_id}") | ||
pass | ||
|
||
|
||
def run_keep_sync(email, password, with_download_gpx=False): | ||
def run_keep_sync(email, password, keep_sports_data_api, with_download_gpx=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
run_page/keep_sync.py
Outdated
parser.add_argument( | ||
"--with-gpx", | ||
dest="with_gpx", | ||
action="store_true", | ||
help="get all keep data to gpx and download", | ||
) | ||
options = parser.parse_args() | ||
run_keep_sync(options.phone_number, options.password, options.with_gpx) | ||
for api in options.sync_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for _types in options.sync_types
.github/workflows/run_data_sync.yml
Outdated
run: | | ||
python run_page/keep_to_strava_sync.py ${{ secrets.KEEP_MOBILE }} ${{ secrets.KEEP_PASSWORD }} ${{ secrets.STRAVA_CLIENT_ID }} ${{ secrets.STRAVA_CLIENT_SECRET }} ${{ secrets.STRAVA_CLIENT_REFRESH_TOKEN }} --sync-types running cycling hiking | ||
# If you only want to sync `type running` modify args --sync-types running, default script is to sync all data (rides, hikes and runs). | ||
# If you only want to sync data from keep to strava, and the running page display the data from strava, you need to change the "if: env.RUN_TYPE == 'keep_to_strava'" -> "if: env.RUN_TYPE == 'strava'" | ||
# If you only want to sync data from keep to strava, and the running page display the data from strava, you need to change the "if: env.RUN_TYPE == 'keep_to_strava_sync'" -> "if: env.RUN_TYPE == 'strava'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the step of changing to 'strava' outdated and unnecessary in the latest version of the code? Please update the comments
* refs/heads/upstream: add keep2strava (yihong0618#653) Add data to 'running page runners' table (yihong0618#656) style: use Tailwind CSS (yihong0618#655) feat: fill country (yihong0618#654) docs: add runner (yihong0618#651) doc: add new runner docs: update README (yihong0618#650) docs: add a comment in `src/utils/consts.ts` to remind about security considerations regarding the `MAPBOX_TOKEN` (yihong0618#644) fix: no package manager fix: tachyons-sass import error (yihong0618#639) Update README.md (yihong0618#638) Add proxy config, add keep config, change npm registry, use yarn (yihong0618#634) chore: fix typo (yihong0618#633) # Conflicts: # README-CN.md # README.md # pnpm-lock.yaml # src/components/Layout/style.module.scss # src/components/LocationStat/index.tsx # src/components/RunMap/index.tsx # src/components/RunMap/style.module.css # src/components/RunTable/RunRow.tsx # src/components/SVGStat/index.tsx # src/components/YearStat/index.tsx # src/styles/variables.scss # src/utils/const.ts # src/utils/utils.ts
* yihong0618-master: (33 commits) Fix codoon and Upload tcx_to_garmin (yihong0618#662) Update README-CN.md (yihong0618#663) add keep2strava (yihong0618#653) Add data to 'running page runners' table (yihong0618#656) style: use Tailwind CSS (yihong0618#655) feat: fill country (yihong0618#654) docs: add runner (yihong0618#651) doc: add new runner docs: update README (yihong0618#650) docs: add a comment in `src/utils/consts.ts` to remind about security considerations regarding the `MAPBOX_TOKEN` (yihong0618#644) fix: no package manager fix: tachyons-sass import error (yihong0618#639) Update README.md (yihong0618#638) Add proxy config, add keep config, change npm registry, use yarn (yihong0618#634) chore: fix typo (yihong0618#633) chore: Update package manager and ignore requirements-dev.txt (yihong0618#632) add run.drink.cafe (yihong0618#628) feat: coros sync (yihong0618#623) Fix README typo (yihong0618#624) fix: ts type error (yihong0618#622) ...
* refs/heads/upstream: (71 commits) Add laqieer's running page to README (yihong0618#672) chore: add raycast extension support (yihong0618#670) add </details> (yihong0618#669) fix the deprecation warning (yihong0618#668) Fix codoon and Upload tcx_to_garmin (yihong0618#662) Update README-CN.md (yihong0618#663) add keep2strava (yihong0618#653) Add data to 'running page runners' table (yihong0618#656) style: use Tailwind CSS (yihong0618#655) feat: fill country (yihong0618#654) docs: add runner (yihong0618#651) doc: add new runner docs: update README (yihong0618#650) docs: add a comment in `src/utils/consts.ts` to remind about security considerations regarding the `MAPBOX_TOKEN` (yihong0618#644) fix: no package manager fix: tachyons-sass import error (yihong0618#639) Update README.md (yihong0618#638) Add proxy config, add keep config, change npm registry, use yarn (yihong0618#634) chore: fix typo (yihong0618#633) chore: Update package manager and ignore requirements-dev.txt (yihong0618#632) ... # Conflicts: # run_page/gpxtrackposter/track.py # run_page/keep_sync.py
Adds keep2strava features and support for multiple sport types
Only provide the ability to sync data from Keep's multiple sport types to Strava's corresponding sport types, to help those who use multiple devices like me. The web page presentation still uses Strava (or refer to nike_to_strava_sync.py to modify it to suit you). #167 #478 #600 #62 #646 #172
Specifics:
My own best practices: