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

Enable xges unit tests on all platform/python versions #894

Merged

Conversation

thiblahute
Copy link
Contributor

After #873 was merged, this enables xges adapter tests on the same platforms/python version as other adapters.

@thiblahute thiblahute force-pushed the xges_test_windows_py2 branch 4 times, most recently from 1ca9328 to 94df91c Compare February 23, 2021 13:40
@thiblahute
Copy link
Contributor Author

cc @ssteinbach

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

Nice, just one question about the import. Thanks for the quick turnaround time!

setup.py Outdated
@@ -54,6 +54,7 @@ class _Ctx(object):

INSTALL_REQUIRES = [
'pyaaf2==1.4.0',
'future',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this to enable the builtins module only in python2? If so note that on line 59 there are some python2 specific dependencies, this could go there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is needed for both python2 and python3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And sorry to not answer last week, I was away. Also I think it was simpler to do it the way we did it in the end :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment saying "# enables the builtins module on python2"

@ssteinbach ssteinbach added this to the Public Beta 14 milestone Feb 23, 2021
Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

My one request is that this gets a comment in the setup.py noting that this is for the builtins module specifically used by the XGES adapter. Otherwise looks great, thanks for the quick turnaround time!

Fixing minor confusion between int and longs
@ssteinbach ssteinbach merged commit b06d05b into AcademySoftwareFoundation:master Feb 24, 2021
@thiblahute
Copy link
Contributor Author

Thanks for the review!

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