azure-sdk-for-python icon indicating copy to clipboard operation
azure-sdk-for-python copied to clipboard

[Pyamqp] Intial TODOS Clean Up

Open kashifkhan opened this issue 3 years ago • 1 comments

  • ws.close on async does close the socket in the library -> link
  • I kept the py2 comment in _transport.py about from os import set_cloexec. Looks like its not in py3 ( yet ) and that import will throw an ImportError across Windows, Mac & Linux.
  • Rejecting links on a ValueError in _incoming_attach. Is that the proper way?

kashifkhan avatar Aug 09 '22 22:08 kashifkhan

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-eventhub

azure-sdk avatar Aug 09 '22 22:08 azure-sdk

In general, I was thinking that we could keep a separate file for outlining negative tests/any tests that we'll want to add in for testing our TODOS. (maybe under something like tests/todo_tests.py?) And in each TODO PR, we can update this file with a comment of the test/pseudocode for the test/something similar.

Since the actual tests will be added to main, but all of our TODO changes will be in pyamqp, I think it would be helpful to be able to couple these TODO test outlines with their corresponding changes. This way, we can clearly track all the tests we need to add, rather than retrospectively figuring out which TODO changes need to be tested and how exactly to test them.

Thoughts?

I like the idea, we can include this in either the PR that fixes those groupings of TODOs or in the Onenote with a linkout to the PR

kashifkhan avatar Aug 17 '22 18:08 kashifkhan