[migration2viatot2.0]: Migrated Supermetrics #671

fdelgadodyvenia · 2023-04-18T10:53:06Z

Summary

Supermetrics Migration

Importance

Migration to viadot 2.0

Checklist

This PR:

Adds source, unit tests, integration test
Changes into config.py: issue
Config.py/get_source_config within Viadot2 possible bug #670

djagoda881

PR Looks good but needs some changes in dostrings and adding some unit tests. Also missing is an entry in the CHANGELOG.md file and __init___.py in source folder.

djagoda881 · 2023-04-18T13:56:00Z

viadot/sources/supermetrics.py

+from ..utils import handle_api_response
+from .base import Source
+
+


Add please credentials definitions using Pydantic as in the example below

viadot/viadot/sources/databricks.py

Line 22 in 9239fcb

class DatabricksCredentials(BaseModel):

✅ [migration2viatot2.0]: Migrated Supermetrics

djagoda881 · 2023-04-18T14:05:59Z

viadot/sources/supermetrics.py

+
+    @classmethod
+    def get_params_from_api_query(cls, url: str) -> Dict[str, Any]:
+        """Returns parmeters from API query in a dictionary"""


I think we can standardize docstrings as in other sources

viadot/viadot/sources/redshift_spectrum.py

Line 109 in 9239fcb

Deletes table from AWS Glue database and related file from Amazon S3, if specified.

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:08:04Z

viadot/sources/supermetrics.py

+        return params_d
+
+    @classmethod
+    def from_url(cls, url: str, credentials: Dict[str, Any] = None):


Missing docstring

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:08:32Z

viadot/sources/supermetrics.py

+        return df
+
+    def query(self, params: Dict[str, Any]):
+        self.query_params = params


Missing docstring

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:08:55Z

viadot/sources/supermetrics.py

+        return obj
+
+    def to_json(self, timeout=(3.05, 60 * 30)) -> Dict[str, Any]:
+        """Download query results to a dictionary.


I think we can standardize docstrings as in other sources

viadot/viadot/sources/redshift_spectrum.py

Line 109 in 9239fcb

Deletes table from AWS Glue database and related file from Amazon S3, if specified.

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:09:18Z

viadot/sources/supermetrics.py

+
+    @classmethod
+    def _get_col_names_other(cls, response: dict) -> List[str]:
+        """Returns list of columns names (to Google Analytics use  _get_col_names_google_analytics ()"""


I think we can standardize docstrings as in other sources

viadot/viadot/sources/redshift_spectrum.py

Line 109 in 9239fcb

Deletes table from AWS Glue database and related file from Amazon S3, if specified.

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:09:23Z

viadot/sources/supermetrics.py

+        return columns
+
+    def _get_col_names(self) -> List[str]:
+        """Returns list of columns names"""


I think we can standardize docstrings as in other sources

viadot/viadot/sources/redshift_spectrum.py

Line 109 in 9239fcb

Deletes table from AWS Glue database and related file from Amazon S3, if specified.

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:12:01Z

viadot/sources/supermetrics.py

+            if_empty (str, optional): What to do if query returned no data. Defaults to "warn".
+
+        Returns:
+            pd.DataFrame: the DataFrame containing query results


Missing dot . and it starts with a lowercase letter

✅ Added changes to supermetrics source/test

djagoda881 · 2023-04-18T14:27:04Z

tests/unit/test_supermetrics.py

+    assert columns == [
+        "Date",
+        "View",
+        "M-site_TOTAL: Bounces Landing",


I think you can add some more unit tests for functions like to_json(), _get_col_names_other(), to_df()

✅ [migration2viatot2.0]: Migrated Supermetrics

djagoda881 · 2023-04-18T14:28:46Z

viadot/config.py

@@ -64,7 +64,8 @@ def get_source_config(key, config=CONFIG):
    if source_configs is not None:
        for source_config in source_configs:
            if key in source_config.keys():
-                return source_configs[source_configs.index(source_config)][key]
+                # return source_configs[source_configs.index(source_config)][key] OBS!!!!!!!!!!!!!


Please add this to CHANGELOG.md with issue number

Also add/fix a test (in test_config.py)

eSlider · 2023-04-20T14:19:30Z

tests/unit/test_supermetrics.py

+        ],
+        "max_rows": 1,
+    }
+    dict_ = s.query(google_ads_params).to_json()


Is adding a dash or sub-dash at the end conventional?

dash added to distinguish python "dict" from our variable.

@burzekj, the dict contains supermetrics, whould it not be better to use the supermetrics for the naming the dict? Otherways, if we need some improvement on the test function, an we get another dict_, should it be named as dict__?

Name dict_ changed
✅ [migration2viatot2.0]: Migrated Supermetrics

eSlider · 2023-04-20T14:21:52Z

tests/unit/test_supermetrics.py

+
+
+def test__to_df():
+    # Create the query


It is often repeated about query creation, although not everywhere, but it is better to describe what is going to be tested in the function annotation.

✅ Added changes to supermetrics source/test

eSlider · 2023-04-20T14:39:31Z

viadot/sources/supermetrics.py

+
+    def to_json(self, timeout=(3.05, 60 * 30)) -> Dict[str, Any]:
+        """
+        Download query results to a dictionary.


Maybe to_json means returning object himself transformed into JSON?

Default timeout value is not human readable and can be transpiled to in the method description.

✅ Added changes to supermetrics source/test

eSlider · 2023-04-20T14:46:21Z

viadot/sources/supermetrics.py

+        Args:
+            if_empty (str, optional): What to do if query returned no data. Defaults to "warn".
+
+


At start, in the annotations(docstring), there were no spaces between text and end, then later they appeared, but one at the end. Now there are two. Should we pass to one format?

✅ Added changes to supermetrics source/test

eSlider · 2023-04-21T10:00:37Z

tests/unit/test_supermetrics.py

+}
+
+
+def test___get_col_names_other():


Function naming can be more human readable:
test get column names other vs. test getting other column names

functions names depends on source functions.
Added comments to be more human readable.
✅ Added changes to supermetrics source/test

eSlider · 2023-04-25T09:53:23Z

viadot/sources/supermetrics.py

+
+    def query(self, params: Dict[str, Any]):
+        """
+        Returns the query with the credentials info.


The name of the function/method should describe what it does, for example, "Query the database using parameters." Then it should describe what parameters can be set and which of them are the default parameters. As example - api_key parameter is missed in the description.

Returned values should be described on bottom of docsting, like with Args:, but Returns:

At this point its impossible to change the name of the functions - function called in other files.
Added docstring
✅ Added changes to supermetrics source/test

trymzet · 2023-05-09T08:00:04Z

@fdelgadodyvenia Please keep 1 PR to 1 new feature (move the other connector to a separate PR)

trymzet

Added my comments

trymzet · 2023-05-09T08:03:10Z

viadot/sources/velux_club.py

+from .base import Source
+
+# Customed Errors!
+class Source_NOK(Exception):


Exceptions in Python should be in CamelCase https://peps.python.org/pep-0008/#exception-names

Pls add docstrings to explain what the exception is for

velux-club removed from this PR
[Supermetrics]: Removed files from

CHANGELOG.md

trymzet · 2023-05-09T08:05:21Z

tests/integration/test_supermetrics.py

+        "max_rows": 1,
+    }
+    df = s.query(google_ads_params).to_df()
+    assert df.count()[0] > 0


if you want to check the number of rows, you can just do assert len(df) == 1

Changed the assert of the function output
[supermetrics-migration]: Done Pull Request Corrections

viadot/sources/velux_club.py

…yvenia/viadot into supermetrics_migration

trymzet · 2023-05-25T10:45:49Z

@fdelgadodyvenia / @burzekj I see you're adding commits but not replying in conversations, can you pls make sure to answer each comment and point to relevant commit/place in commit so reviewers can verify and resolve conversations 1 by 1?

wrong file for this branch

Diego-H-S · 2024-03-13T10:36:01Z

Hey, another PR that must be reviewed.
This one with BigQuery and salesforce PRs is a very important PR to be reviewed to interact with Jackub as much as possible.

trymzet · 2024-09-26T09:20:15Z

Closing since there is a newer PR for this #1054

[migration2viatot2.0]: Migrated Supermetrics

af8444a

fdelgadodyvenia requested review from djagoda881 and trymzet April 18, 2023 10:53

djagoda881 reviewed Apr 18, 2023

View reviewed changes

[supermetrics-migration]: Done Pull Request Corrections

8c49474

eSlider reviewed Apr 21, 2023

View reviewed changes

eSlider reviewed Apr 25, 2023

View reviewed changes

fdelgadodyvenia added 2 commits May 8, 2023 10:08

[Supermetrics]: Restablished Config.py

e0674b2

[velux_club]: Introduced Velux_Club Connector

184df69

trymzet reviewed May 9, 2023

View reviewed changes

cgildenia and others added 7 commits May 9, 2023 10:02

Corrected Historical_Too_Old message

2327d02

[velux_club]: Completed connector, source, task and flow

a9112aa

Merge branch 'supermetrics_migration' of https://github.com/fdelgadod…

98a8611

…yvenia/viadot into supermetrics_migration

[velux-connector]: format corrections!

43a8810

[Supermetrics]: Update tests

0fcd640

[Supermetrics]: Re-Correction!

2f95c74

✅ Added changes to supermetrics source/test

72fd8db

fdelgadodyvenia and others added 5 commits May 26, 2023 13:39

[Supermetrics]: Removed files from

a35167d

Delete test_velux_club.py

76b5404

wrong file for this branch

✅ [migration2viatot2.0]: Migrated Supermetrics

0a1e722

CHANGELOG.md correction

c1bd962

🎨 fix error handlings, added metadata decorator

9539c03

Diego-H-S requested review from djagoda881, burzekj, trymzet and eSlider March 13, 2024 10:33

Merge branch '2.0' into supermetrics_migration

7b773f1

trymzet mentioned this pull request Aug 30, 2024

Add Supermetrics source, tasks, and flow #1014

Closed

5 tasks

trymzet closed this Sep 26, 2024

		from ..utils import handle_api_response
		from .base import Source

		Args:
		if_empty (str, optional): What to do if query returned no data. Defaults to "warn".

[migration2viatot2.0]: Migrated Supermetrics #671

[migration2viatot2.0]: Migrated Supermetrics #671

Conversation

fdelgadodyvenia commented Apr 18, 2023

Summary

Importance

Checklist

djagoda881 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eSlider Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eSlider Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trymzet commented May 9, 2023

trymzet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trymzet commented May 25, 2023 • edited Loading

Diego-H-S commented Mar 13, 2024

trymzet commented Sep 26, 2024

eSlider Apr 20, 2023 •

edited

Loading

eSlider Apr 21, 2023 •

edited

Loading

trymzet commented May 25, 2023 •

edited

Loading