Skip to content
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

Added support in recurring group name in files. #45

Merged
merged 9 commits into from
Mar 20, 2022

Conversation

yochaytz
Copy link
Contributor

No description provided.

@AKuederle
Copy link
Collaborator

Thanks for the merge request!

In general i think this is a feasible workaround. the only thing I am thinking about is, if it would make sense to change the naming schema for the duplicated groupnames. Maybe {groupname}_{group_id}? How does the groupid usually look like?

@AKuederle AKuederle linked an issue Mar 10, 2022 that may be closed by this pull request
@AKuederle
Copy link
Collaborator

@MattiasFredriksson does this interfere with #43 in any way?

c3d/c3d.py Outdated Show resolved Hide resolved
c3d/c3d.py Outdated Show resolved Hide resolved
c3d/c3d.py Outdated Show resolved Hide resolved
@MattiasFredriksson
Copy link
Contributor

MattiasFredriksson commented Mar 10, 2022

@AKuederle AKuederle There should be a conflict but there are no functional differences (the KeyError message was updated to be more descriptive).

Otherwise I'm wondering if the name update should be done in this fashion since this is a public method. Not throwing KeyErrors when external code adds a Group could cause silent errors/bugs as it would allow duplicates to be created 'unintentionally'. However, resolving such issues could be done in the future as it would probably complicate the solution a bit.

The standard string formatting currently used is on the form:
'Repeated group name {} modified to {}'.format(name, new_name)

@AKuederle
Copy link
Collaborator

I agree! My suggestion would be to hide the overwrite behind a parameter that is False by default

@AKuederle
Copy link
Collaborator

I made the changes I suggested above:

  1. Made the overwrite optional
  2. Changed the rename schmema to be more predictable

@friggog What do you think?

@AKuederle AKuederle mentioned this pull request Mar 12, 2022
@AKuederle AKuederle requested a review from friggog March 12, 2022 16:27
@AKuederle AKuederle merged commit d8ea2f9 into EmbodiedCognition:master Mar 20, 2022
@AKuederle
Copy link
Collaborator

@yochaytz Thanks again for the contribuition! I will try to make a new release later today containing this fix.

And @friggog and @MattiasFredriksson thanks for the reviews and the input!

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.

Parsing valid c3d file fails
4 participants