-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Make 'oca-port' available as a Python package + JSON output #26
Conversation
66580ba
to
31a1179
Compare
This class consolidates all the parameters required by oca-port to run and expose the features.
MigrateAddon: if an addon could be migrated on the target branch, the process will return 100 as exit code. PortAddonPullRequest: if some pull requests/commits could be ported for the given addon, the process will return 110 as exit code.
And an 'Output' mixin used by all the classes that need to print/render data to the user. If oca-port is not used through the CLI but instead through its API, messages won't be printed on the std output. Thanks to this change, tests can be easily added now.
31a1179
to
30bbc6f
Compare
5cd04eb
to
71b667d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! We need more tests tho 😜
@@ -38,15 +38,19 @@ | |||
in one branch and creating a draft PR against the upstream repository with all | |||
of them. | |||
""" | |||
import pathlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not-so-nitpicking: can we please move everything out of __init__.py
file? 🙏
Maybe in another round... 😉
commit = repo.index.commit(f"[FIX] {self._settings['addon']}: fix dependency") | ||
return commit.hexsha | ||
|
||
def tearDown(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO you should use https://click.palletsprojects.com/en/8.1.x/testing/#file-system-isolation
app.run() | ||
except SystemExit as exc: | ||
# exit code 100 means the module could be migrated | ||
self.assertEqual(exc.args[0], 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those exceptions should have a code
attribute IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but that's the std SystemExit
exception that accepts only one value, like the sys.exit(...)
function.
Not sure it deserves a dedicated exception inheriting from SystemExit
here for such little need in a test?
82115ff
to
a103c93
Compare
This PR has the |
67b00fb
to
e57b65f
Compare
It currently supports only the 'json' value.
e57b65f
to
6cf6f7d
Compare
@sebalix what is left here to merge? |
Several things here (see commits):
App
class (used by all subsequent classes to not duplicate parameters everywhere)oca_port
Python package (and ease the writing of tests! So some basic tests added)--non-interactive
option, 3 exit codes can be returned:0
: all good, nothing to migrate or port (the Git history is the same on the source & target branches)100
: the module could be migrated on the target branch110
: some commits/pull requests could be ported on the target branch for the given module--output
to return a parse-able result (working both in CLI mode or through the API) which:--non-interactive
mode automatically--fetch
option to force the fetch of source & target branches--fetch
to update the remote branches (or update them by her/himself)Example:
Module already migrated but some commits could be ported:
Module alreay migrated and nothing to port:
TODO:
This PR implements (partially) issue #17