Airtable connector #631
Airtable connector #631
Conversation
WalkthroughThis change introduces the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (4)
- pandasai/connectors/init.py (2 hunks)
- pandasai/connectors/airtable.py (1 hunks)
- pandasai/connectors/base.py (1 hunks)
- tests/connectors/test_airtable.py (1 hunks)
Files skipped from review due to trivial changes (1)
- pandasai/connectors/init.py
Additional comments (Suppressed): 5
tests/connectors/test_airtable.py (2)
9-21: The
setUpmethod is used to set up the testing environment before each test method is run. Here, an instance ofAirtableConnectorConfigandAirtableConnectoris created. This is a good practice as it reduces code duplication in each test method.22-28: This test case checks the constructor and properties of the
AirtableConnectorclass. It verifies that the_configattribute is correctly set and that the_sessionattribute is an instance ofrequests.Session. It also checks that theAuthorizationheader is correctly set in the_sessionattribute. This is a good practice as it ensures that the class is correctly initialized.pandasai/connectors/base.py (2)
- 74-76: The
tokenandhttpPathattributes are already defined in the old hunk. If these are not specific toAirtableConnectorConfig, consider removing them to avoid duplication.- token: str - httpPath: str
- 77-83: The
AirtableConnectorConfigclass is well defined withtokenandbaseIDattributes. Ensure that thetokenandbaseIDare properly validated before being used in theAirtableConnectorclass.pandasai/connectors/airtable.py (1)
- 1-74: The
AirtableConnectorclass is well-structured and follows the SOLID principles. However, there are a few areas that could be improved for better error handling and maintainability.
| @patch("pandasai.connectors.AirtableConnector._connect_load") | ||
| @patch("pandas.DataFrame") | ||
| def test_connect_load(self,_connect_load,mock_dataframe): | ||
| response = _connect_load(self.config) | ||
| self.assertEqual(type(self.connector._response),mock_dataframe()) | ||
|
|
There was a problem hiding this comment.
This test case checks the _connect_load method of the AirtableConnector class. It verifies that the _response attribute is an instance of pandas.DataFrame. However, the _connect_load method is patched but not used in the test. Instead, the self.config is passed directly to _connect_load which is incorrect. The _connect_load method should be called on the self.connector instance. Here is the corrected code:
- response = _connect_load(self.config)
+ response = self.connector._connect_load()| @patch("pandasai.connectors.AirtableConnector.head") | ||
| @patch("pandas.DataFrame") | ||
| def test_head(self,mock_head,mock_dataframe): | ||
| response = mock_head() | ||
| self.assertEqual(type(response),mock_dataframe()) |
There was a problem hiding this comment.
This test case checks the head method of the AirtableConnector class. It verifies that the returned response is an instance of pandas.DataFrame. However, similar to the previous test case, the mock_head method is patched but not used in the test. Instead, the head method should be called on the self.connector instance. Here is the corrected code:
- response = mock_head()
+ response = self.connector.head()| def __init__( | ||
| self, | ||
| baseid : Optional[str] = None, | ||
| bearer_token : Optional[str] = None, | ||
| table_name : Optional[str] = None, | ||
| config : Union[AirtableConnectorConfig,dict] = None | ||
| ): |
There was a problem hiding this comment.
The constructor of AirtableConnector accepts both individual parameters and a config object. This could lead to confusion and potential bugs if both are provided but with conflicting values. Consider simplifying the constructor to accept only a config object. This would make the function signature simpler and more maintainable.
| if not bearer_token and not config: | ||
| raise ValueError( | ||
| "You must specify bearer token for authentication or a config object." | ||
| ) | ||
| if not baseid and not config.baseID: | ||
| raise ValueError( | ||
| "You must specify baseId or a proper config object." | ||
| ) |
There was a problem hiding this comment.
The error messages here are not very informative. It would be better to specify which parameter is missing in the error message. This would make debugging easier.
| if not isinstance(config,AirtableConnectorConfig): | ||
| if not config : | ||
| config = {} | ||
| if table_name : | ||
| config['table'] = table_name | ||
|
|
||
| airtable_config = AirtableConnectorConfig(**config) | ||
| else : | ||
| airtable_config = config |
There was a problem hiding this comment.
The handling of the config parameter is a bit convoluted. It would be clearer to handle the config parameter separately depending on its type. This would make the code easier to read and maintain.
| _response = self._session.get(f"https://api.airtable.com/v0/{config['baseID']}/{config['table']}") | ||
| if _response.status_code == 200 : | ||
| self._response = pd.read_json(_response.json()) | ||
| else : | ||
| raise ValueError( | ||
| "Please verify authentication details , baseid or table_name are correct ." | ||
| ) |
There was a problem hiding this comment.
The error handling here could be improved. If the status code is not 200, it would be helpful to include the status code and the response message in the error. This would make debugging easier.
| def head(self): | ||
| """ | ||
| Return the head of the table that the connector is connected to. | ||
|
|
||
| Returns : | ||
| DatFrameType: The head of the data source that the conector is connected to . | ||
| """ | ||
| return self._response.head() | ||
|
|
There was a problem hiding this comment.
The head method assumes that _response is not None and that it is a DataFrame. It would be safer to check that _response is not None and that it is a DataFrame before calling head on it. This would prevent runtime errors.
Here are the suggested changes:
def __init__(
self,
- baseid : Optional[str] = None,
- bearer_token : Optional[str] = None,
- table_name : Optional[str] = None,
config : Union[AirtableConnectorConfig,dict] = None
):
- if not bearer_token and not config:
- raise ValueError(
- "You must specify bearer token for authentication or a config object."
- )
- if not baseid and not config.baseID:
- raise ValueError(
- "You must specify baseId or a proper config object."
- )
- if not isinstance(config,AirtableConnectorConfig):
- if not config :
- config = {}
- if table_name :
- config['table'] = table_name
-
- airtable_config = AirtableConnectorConfig(**config)
- else :
- airtable_config = config
+ if isinstance(config, dict):
+ airtable_config = AirtableConnectorConfig(**config)
+ elif isinstance(config, AirtableConnectorConfig):
+ airtable_config = config
+ else:
+ raise ValueError("Invalid config parameter. Expected a dict or an AirtableConnectorConfig instance.")
_response = self._session.get(f"https://api.airtable.com/v0/{config['baseID']}/{config['table']}")
if _response.status_code == 200 :
self._response = pd.read_json(_response.json())
else :
- raise ValueError(
- "Please verify authentication details , baseid or table_name are correct ."
- )
+ raise ValueError(f"Failed to connect to Airtable. Status code: {_response.status_code}, message: {_response.text}")
def head(self):
"""
Return the head of the table that the connector is connected to.
Returns :
DatFrameType: The head of the data source that the conector is connected to .
"""
- return self._response.head()
+ if self._response is not None and isinstance(self._response, pd.DataFrame):
+ return self._response.head()
+ else:
+ raise ValueError("No data loaded. Please call _connect_load first.")
Implemented Airtable connector with the help of official docs.
Required parameters according to new policy of airtable . Authorization token is used for authentication and
baseidandtable_namerequired to connect and load data.Summary by CodeRabbit
New Feature:
AirtableConnectormodule to thepandasaipackage. This module allows users to connect to Airtable records, authenticate, and perform operations like retrieving the head of the table.Refactor:
AirtableConnectorConfigthat extendsBaseConnectorConfigto handle the connector configuration for Airtable.Test:
TestAirTableConnectorto validate the functionality of theAirtableConnectorclass. The tests cover the constructor, properties,connect_loadmethod, andheadmethod.