Skip to content

[core] add six and replace custom compat functions#751

Merged
brettlangdon merged 8 commits into
0.18-devfrom
brettlangdon/six
Dec 7, 2018
Merged

[core] add six and replace custom compat functions#751
brettlangdon merged 8 commits into
0.18-devfrom
brettlangdon/six

Conversation

@brettlangdon

@brettlangdon brettlangdon commented Nov 30, 2018

Copy link
Copy Markdown
Member

This PR does a few things:

  1. Adds six as a dependency and aliases our ddtrace.compat properties to the appropriate six ones
  2. Inlines compat_async.py into compat.py

@brettlangdon

Copy link
Copy Markdown
Member Author

#755 is proving to be a bit more difficult, and it is exposing some potential issues with this vendoring approach. We should consider both PRs in tandem on whether/if/how we should be doing vendoring.

@brettlangdon brettlangdon changed the title [core] vendor six and replace custom compat functions [core] add six and replace custom compat functions Dec 6, 2018

@Kyle-Verhoog Kyle-Verhoog left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes are good 👍. I'm not convinced that our compat functions are 100% equivalent to the six alternatives. While our tests pass, I wonder whether we should do some more thorough testing since these are pretty substantial changes.

@brettlangdon

Copy link
Copy Markdown
Member Author

From what I can tell, a few of them looked like copy/pasta.

I agree, but I am also willing to bet that sixs implementations are more battle tested, and "correct".

@brettlangdon
brettlangdon merged commit 23fb12b into 0.18-dev Dec 7, 2018
@Kyle-Verhoog
Kyle-Verhoog deleted the brettlangdon/six branch March 19, 2020 23:25
@P403n1x87 P403n1x87 mentioned this pull request Jun 6, 2023
14 tasks
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