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

Update to FIT SDK v20.35 #31

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

Update to FIT SDK v20.35 #31

wants to merge 1 commit into from

Conversation

liamjm
Copy link
Contributor

@liamjm liamjm commented Jun 26, 2017

This required a few tweaks to generate_profile.py.

There are a large number of diffs in the final profile.py. This is caused by the presence of more "group_name" in the message list, which affect the key in the comparator for the sorted() function.
This is annoying as it makes it hard to really diff. Let me know if you have other thoughts on this.

@liamjm
Copy link
Contributor Author

liamjm commented Sep 27, 2017

@dtcooper Anyone got time to look at this one?

@pR0Ps
Copy link
Collaborator

pR0Ps commented Oct 4, 2017

@liamjm I've just had a look but it's pretty much impossible to do a thorough full review of a +/- 5000 change diff.

Some questions:

  • I noticed is that all the units fields are now all unicode strings. Was this done on purpose? Does it affect anything in the parsing/data processing?
  • Did all the tests pass using the final profile.py?

A few stray thoughts on making diffs easier going forward:

  • Most of the elements that are output by the generate_profile.py script sort by name. However, MessageInfo, MessageList both sort their fields dictionary attributes by id (after group name). Would changing this to sort by name instead result in more manageable diffs (ie. would the numbers ever change while referring to the same data)?
  • Removing the MessageList group name sorting from the generated profile.py would probably make future diffs much simpler. This would have an impact on human readability but does a file full of autogenerated struct definitions really need to be human-readable?

@dtcooper thoughts?

@dtcooper
Copy link
Owner

I trust your judgement, @pR0Ps. But I would like to see this file programatically generated, or if things need to happen by hand, then a clear documented process to generate the profile.

@liamjm
Copy link
Contributor Author

liamjm commented Oct 14, 2017

Hey,

This change was generated by the generate_profile.py, which required a few tweaks to parse the latest SDK version. But yes, it is crazy big and unreasonable to expect manual review.
A few more thoughts going forward:

  • let's add running of the tests to the project to protect against regressions.
  • I'll try to get v20.33 of the SDK (the source of the current version) and re-generate profile.py to ensure that no changes are being introduces by my environment.
  • IMHO sorting should be done to reduce differences. We just need to work out the most stable property to sort on. To the best of my recall, many of the changes were caused by the change in group name, but I can dig into this more.

@liamjm
Copy link
Contributor Author

liamjm commented Jan 6, 2018

Quick update:

  • Adding of the test files is in Create config files for tox and travis-CI #40
  • The previous versions of the SDK aren't publicly accessible (though I have asked https://www.thisisant.com/forum/viewthread/6968/). This means I'm unable to re-produce the v20.33 Profile. I have generated the v20.54 by using python3 and there are much less diffs. I'll update the docs to explain this.
  • We'll need to work out the best way to produce stable sorting of the field, I've not looked at that yet.

@dtcooper
Copy link
Owner

dtcooper commented Jan 6, 2018 via email

@xmedeko
Copy link
Contributor

xmedeko commented Mar 13, 2018

What about to split profile.py file to more files: profile_types.py and profile_messages.py (or just the old profile.py to keep backward compatibility.) Then, it would be a bit simpler to track commit changes.

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.

4 participants