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

Factor out annotation-related functionality into a separate module #439

Merged
merged 4 commits into from
Sep 2, 2021
Merged

Factor out annotation-related functionality into a separate module #439

merged 4 commits into from
Sep 2, 2021

Conversation

IRDonch
Copy link

@IRDonch IRDonch commented Aug 31, 2021

Summary

Annotations are only tangentially related to extractors, so I don't think they belong in extractor.py. extractor.py is also a large module, so removing a chunk of it makes it easier to navigate.

How to test

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@IRDonch
Copy link
Author

IRDonch commented Aug 31, 2021

@zhiltsov-max This PR is incomplete (I haven't updated the examples in the documentation yet). Let me know if you agree with this refactoring and if you do, I'll finish it.

@zhiltsov-max
Copy link
Contributor

Thought of it. It does not really matter until we have stable and documented API, but this breaks the library API.

@IRDonch
Copy link
Author

IRDonch commented Aug 31, 2021

It does not really matter until we have stable and documented API, but this breaks the library API.

I could add compatibility exports if that's your concern. Although I kinda figured that with the versioning changes we're pretty much breaking all compatibility anyway. :P

@IRDonch
Copy link
Author

IRDonch commented Sep 1, 2021

So what should I do? Should I add the compatibility exports?

@zhiltsov-max
Copy link
Contributor

Let's put off the PR till we introduce the stable API after the versioning release.

@IRDonch
Copy link
Author

IRDonch commented Sep 1, 2021

Let's put off the PR till we introduce the stable API after the versioning release.

I don't think that's a good course of action; it would be much harder to bring the PR up-to-date at that point (which could be long into the future) than it would be to just add compatibility exports now and then remove them later. Plus, by merging this now we can improve the navigability of extractor.py in the interim (I initiated this refactoring in the first place because I kept getting lost in it).

@zhiltsov-max
Copy link
Contributor

zhiltsov-max commented Sep 1, 2021

Ok, then add compatibility exports, and it is ready.

Roman Donchenko added 2 commits September 1, 2021 19:22
Annotations are only tangentially related to extractors, so I don't think
they belong in extractor.py. `extractor.py` is also a large module, so removing
a chunk of it makes it easier to navigate.
@IRDonch IRDonch marked this pull request as ready for review September 1, 2021 16:23
@IRDonch
Copy link
Author

IRDonch commented Sep 1, 2021

OK, done and updated the documentation.

@zhiltsov-max zhiltsov-max merged commit 2db4700 into openvinotoolkit:develop Sep 2, 2021
@IRDonch IRDonch deleted the factor-out-annotations branch September 29, 2021 10:51
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