Skip to content

[DO NOT MERGE] Imports SweatStack library for assessment #3

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AartGoossens
Copy link
Collaborator

@dierickxsimon Can you review this code that I imported from SweatStack?
At the moment, it depends on fitdecode but that can (and should) be easily replaced with the Garmin FIT Python SDK.
I think we should mainly discuss if the interface and how the data is returned (including the "opiniated" column handling) is how we would like it for this new library.

@dierickxsimon
Copy link
Collaborator

Personally I think te best way to design the interface is to something like a composite pattern. Were The a general Fitfile class acts like a container with all the differen components (activity, records and all other fields that can be found inside the fitfile ( see: https://www.fitfileviewer.com/). So when the user invokes a read_fit function, the function would return a fitfile object that the user can then use to filter out the data and data types that the user would need.

About the opinionated column I think it important that there is always an option to get the full data. But I’m also fan of some filter functionality to make it more user friendly for common use cases.

@AartGoossens
Copy link
Collaborator Author

I agree with you that the code should probably be structured in a composite way. However, for usability and intuitiveness, I think this library should at least also offer a read_fit() method that returns a pandas DataFrame: read_{filename}() is the pattern that is used throughout the Python data/pandas ecosystem for methods that return dataframes and I think we should not deviate from that.

If we force read_fit() to make use of a FITFile class that follows the composite pattern, the overhead of offering both interfaces is very minimal. Do you think that could work?

@dierickxsimon
Copy link
Collaborator

dierickxsimon commented Jan 15, 2025

Yes even better, sounds like we are on the same page! Should we also consider functionality for writing fit files? For example workout files.

@AartGoossens
Copy link
Collaborator Author

Writing files would be a really nice feature that is not available in most libraries (also outside of Python), but I would say it's not a top priority.

@AartGoossens
Copy link
Collaborator Author

Shall we create issues for ideas like this (writing FIT files) or would that turn into an issue mess?

@dierickxsimon
Copy link
Collaborator

The issue mess is inevitable. Nevertheless I think it's a good idea to create issues with the ideas we have!

@dierickxsimon
Copy link
Collaborator

dierickxsimon commented Jan 16, 2025

starting an issue (#6) for discussing the project structure further.

@AartGoossens
Copy link
Collaborator Author

Let's close this issue and potentially re-open when we start with the read_fit() interface. I do think the "opiniated interface" is beneficial to read_fit() (but not necessarily the Fitfile class) so we can base it on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants