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.

Sign up for GitHub

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

Jump to bottom

[IMP][9.0] base_external_dbsource: Refactor & Split by source #642

Closed
wants to merge 4 commits into from
Closed

[IMP][9.0] base_external_dbsource: Refactor & Split by source #642

wants to merge 4 commits into from

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Dec 10, 2016 •
edited
Loading

This is a refactoring of the base_external_dbsource module, hopefully allowing for easier addition and more isolation of the independent data sources.

Note that all adapter modules are auto_install to True in order to not break existing installations while still allowing for the modules to be split logically. In v11 we can then add the proper Python depends, which are already in the manifests but commented out.

Due to the above note, I also had to submit all of the modules in one PR. Separating them will break things horribly. Sorry for the gigantic PR

Depends:

@lasley lasley added this to the 9.0 milestone Dec 10, 2016
@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch 4 times, most recently from 5f76e8f to f8781ae Compare December 10, 2016 18:00
@lasley lasley changed the title [d6ea9ec] [IMP] base_external_dbsource: Refactor & Split by source [IMP][9.0] base_external_dbsource: Refactor & Split by source Dec 12, 2016
@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch 12 times, most recently from 39a70a6 to c7d49c8 Compare December 12, 2016 23:57
@lasley
Copy link
Contributor Author

lasley commented Dec 13, 2016 •
edited
Loading

Alright I'm done here. Not sure what's up with Runbot - it's acting like it's not installing the FDB from requirements.txt, but the error isn't replicating on Travis. I'll probably have to look more into that, but this can be reviewed with that & the license change #637 in mind.

@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch from a1abeea to 1ee71c9 Compare December 13, 2016 05:06
lasley
lasley commented Dec 13, 2016
View reviewed changes
base_external_dbsource/README.rst
* Add concept of multiple connection strings for one source (multiple nodes)
* Add a ConnectionEnvironment that allows for the reuse of connections
* Message box should be displayed instead of error in ``connection_test``
* Remove old api compatibility layers (v11)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

* Add CRUD interface (use Elasticsearch module additions as template)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented - still needs tests

@lasley lasley mentioned this pull request Dec 13, 2016
Closed
1 task
@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch 5 times, most recently from 3f3fbb7 to db0f84f Compare December 13, 2016 22:38
@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch 11 times, most recently from adf373b to 59f0362 Compare December 20, 2016 05:39
@lasley
Copy link
Contributor Author

lasley commented Dec 20, 2016

Alright this is good to review; now including a fancy new CRUD interface if adapters should choose to implement them & rebased on 9.0 to fix requirements conflict.

@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch from 59f0362 to f896764 Compare December 20, 2016 06:31
@lasley lasley mentioned this pull request Dec 20, 2016
Merged
1 task
@lasley
[IMP] base_external_dbsource: Refactor & Split by source
06b22d3 
* Heavily refactor code for reusability
* Split all sources into independent modules
* Add more test coverage
* Add CRUD methods
@lasley lasley force-pushed the feature/9.0/base_external_dbsource-refactor branch from f896764 to 06b22d3 Compare December 20, 2016 07:47
@lasley
Copy link
Contributor Author

lasley commented Dec 20, 2016

Rebased now that dependency is merged

@lasley lasley mentioned this pull request Dec 20, 2016
Open
@lasley
Copy link
Contributor Author

lasley commented Dec 28, 2016

@t3ddftw please review

tedsalmon
tedsalmon approved these changes Dec 29, 2016
View reviewed changes
Copy link

@tedsalmon tedsalmon left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

My review items are all just spelling :p

base_external_dbsource/README.rst
server-tools/issues/new?body=module:%20
base_external_dbsource%0Aversion:%20
9.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.
If you spotted it first, help us smashing it by providing a detailed and welcomed feedback.
Copy link

Choose a reason for hiding this comment

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

s/smashing/smash

Copy link
Contributor Author

@lasley lasley Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is part of the template, and while I do agree the wording needs to be changed, it is out of scope here. Proposing a change to the template for this is something I've been meaning to do, you're welcome to as well - https:/OCA/maintainer-tools/blob/master/template/module/README.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OCA/maintainer-tools#251

base_external_dbsource/tests/test_base_external_dbsource.py
self.assertEqual(res, adapter())

def test_remote_browse_asserts_current_table(self):
""" It should raise AssertionError if a table not selected """
Copy link

Choose a reason for hiding this comment

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

s/if a table not/if a table is not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(you can save yourself some effort and just tell me to apply X change to all instances)

base_external_dbsource/tests/test_base_external_dbsource.py
self.assertEqual(res, adapter())

def test_remote_create_asserts_current_table(self):
""" It should raise AssertionError if a table not selected """
Copy link

Choose a reason for hiding this comment

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

s/table not/table is not

base_external_dbsource/tests/test_base_external_dbsource.py
self.assertEqual(res, adapter())

def test_remote_delete_asserts_current_table(self):
""" It should raise AssertionError if a table not selected """
Copy link

Choose a reason for hiding this comment

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

s/table not/table is not

base_external_dbsource/tests/test_base_external_dbsource.py
self.assertEqual(res, adapter())

def test_remote_search_asserts_current_table(self):
""" It should raise AssertionError if a table not selected """
Copy link

Choose a reason for hiding this comment

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

s/table not/table is not

base_external_dbsource/tests/test_base_external_dbsource.py
self.assertEqual(res, adapter())

def test_remote_update_asserts_current_table(self):
""" It should raise AssertionError if a table not selected """
Copy link

Choose a reason for hiding this comment

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

s/table not/table is not

base_external_dbsource_firebird/models/base_external_dbsource.py
import logging

from openerp import api
from openerp import models
Copy link

Choose a reason for hiding this comment

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

Condense this to a single line, IMO.

base_external_dbsource_firebird/models/base_external_dbsource.py


class BaseExternalDbsource(models.Model):
""" It provides logic for connection to an Firebird data source. """
Copy link

Choose a reason for hiding this comment

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

s/an/a

base_external_dbsource_mssql/README.rst

Bugs are tracked on `GitHub Issues <https:/OCA/server-tools/issues>`_.
In case of trouble, please check there if your issue has already been reported.
If you spotted it first, help us smashing it by providing a detailed and welcomed feedback.
Copy link

Choose a reason for hiding this comment

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

s/smashing/smash

@lasley
Copy link
Contributor Author

lasley commented Dec 30, 2016

Thanks for the review @t3ddftw - most points fixed, some responded to.

@lasley lasley mentioned this pull request Dec 30, 2016
Merged
@lasley lasley mentioned this pull request Jan 21, 2017
Closed
@lasley
[FIX] Move DB libs to travis file
86166ac 
* Move libraries that require development headers to travis file to band-aid build
@lasley
Copy link
Contributor Author

lasley commented Jan 24, 2017

Added build fix for OCA/maintainer-quality-tools#427

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 17, 2022
@github-actions github-actions bot closed this May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tedsalmon tedsalmon tedsalmon approved these changes

Assignees
No one assigned
Labels
needs review stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@lasley @tedsalmon