Add the readme.md example dag to example dags folder #681

utkarsharma2 · 2022-08-18T09:47:05Z

Description

What is the current behavior?

Currently, we have examples in the README.md file which are not getting tested as we update the code.

closes: #680

What is the new behavior?

We can move our code to example_dags and refer to that file in README.md so that this file is tested with every change.

Does this introduce a breaking change?

Nope

Checklist

All checks and tests in the CI should pass
Example DAG
Improve the documentation (README, Sphinx, and any other relevant)

sunank200

Isn't this DAG just a subset of https://github.com/astronomer/astro-sdk/blob/main/example_dags/example_transform.py ?

We could just use that. WDYT?

example_dags/calculate_popular_movies.py

feluelle

For me it is not showing the whole code at once. I would have to scroll down to see everything. :/

utkarsharma2 · 2022-08-18T10:10:43Z

For me it is not showing the whole code at once. I would have to scroll down to see everything. :/

@feluelle I didn't see any way to change the size of the code snippet. I'll check and get back.

kaxil · 2022-08-18T13:20:45Z

README.md

-            ),
-        )
-    ```
+    https://github.com/astronomer/astro-sdk/blob/d5aa768b2d4bca72ef98f8d533fe3f99624b172f/example_dags/calculate_popular_movies.py#L1-L37


WDYT about the following instead, thoughts? cc @kentdanas @feluelle @mikeshwe @jwitz

Suggested change

https://github.com/astronomer/astro-sdk/blob/d5aa768b2d4bca72ef98f8d533fe3f99624b172f/example_dags/calculate_popular_movies.py#L1-L37

curl -O https://raw.githubusercontent.com/astronomer/astro-sdk/main/example_dags/calculate_popular_movies.py

jwitz · 2022-08-18T13:46:28Z

README.md

@@ -50,42 +50,7 @@ pip install astro-sdk-python[amazon,google,snowflake,postgres]

 1. Copy the following DAG into a file named `calculate_popular_movies.py` and add it to the `dags` directory of your Airflow project:


I'd actually prefer if we kept the DAG code visible in the README. Even though it's longer, the reader won't have to click into another page/ run a command before they see how the SDK works. We can still keep the other DAG file, but this would serve as the window into what the SDK can do

If we could embed the code it would be the best scenario from both perspectives

jwitz · 2022-08-18T13:47:16Z

README.md

-    from astro.files import File
-    from astro.sql.table import Table
-
-    @aql.transform()


If we add this back to the README as code, it would be great to add some more inline comments about what the code/ SDK is doing here.

tatiana · 2022-09-05T09:52:04Z

README.md

-            ),
-        )
-    ```
+    curl -O https://raw.githubusercontent.com/astronomer/astro-sdk/main/example_dags/calculate_popular_movies.py


@utkarsharma2 why did you favour this approach instead of adding the code snipped with permalink? Is it because the permalink would have the version of the code hard-coded?

I would use the permalink, otherwise we could end up having a 404 (completely missing) in our README.

@tatiana @feluelle initially permalinks was my approach - 21f7bd8#r949124948

But I guess @kaxil felt the alternative would be better. I personally inclined towards the permalinks approach.

refer below link for that discussion - #681 (comment)

I felt that mainly because users would otherwise have to copy/paste it and because of scrolling involved with code-snippet that might result in issues.

However, if all of you think snippet + permalink is a better approach then go for it.

@utkarsharma2 What if we kept both of them? The code snipped & the curl command - so we would meet the requirement from the doc team while also simplifying for those who may want to download it straightway - without having to have the code hard-coded in the README?

@tatiana I think we can keep both if we are able to verify the curl link is working. Just a little worried if move the file somewhere else and the links are broken.

@tatiana Never mind - CI job takes care of that - gaurav-nelson/github-action-markdown-link-check@v1. I'm going ahead with adding both.

Never mind - CI job takes care of that - gaurav-nelson/github-action-markdown-link-check@v1. I'm going ahead with adding both.

Right. That's great! +1 for using main version.

tatiana · 2022-09-06T21:12:18Z

README.md

-        imdb_movies = aql.load_file(imdb_src, imdb_movies)
+   https://github.com/astronomer/astro-sdk/blob/d5aa768b2d4bca72ef98f8d533fe3f99624b172f/example_dags/calculate_popular_movies.py#L1-L37
+
+   Alternatively you can download the file


Suggested change

Alternatively you can download the file

Alternatively, you can download the file by running:

tatiana · 2022-09-06T21:13:18Z

README.md

+4. Ensure that your Airflow environment is set up correctly by running the following commands:

        top_animations = Table(name="top_animation")
        top_animations = top_five_animations(input_table=imdb_movies, output_table=top_animations)
    ```


I didn't fully understand this part. Do we need it?

@tatiana yes, we do, was a typo fixed it.

codecov · 2022-09-06T21:23:42Z

Codecov Report

Merging #681 (815a607) into main (1f5f16c) will not change coverage.
The diff coverage is n/a.

❗ Current head 815a607 differs from pull request most recent head 6274b25. Consider uploading reports for the commit 6274b25 to get more accurate results

@@           Coverage Diff           @@
##             main     #681   +/-   ##
=======================================
  Coverage   93.26%   93.26%           
=======================================
  Files          43       43           
  Lines        1855     1855           
  Branches      232      232           
=======================================
  Hits         1730     1730           
  Misses         97       97           
  Partials       28       28

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

utkarsharma2 requested review from dimberman, tatiana, sunank200, pankajastro, pankajkoti and feluelle as code owners August 18, 2022 09:47

utkarsharma2 changed the title ~~Ref codein md~~ Add the readme.md example dag to example dags folder Aug 18, 2022

sunank200 reviewed Aug 18, 2022

View reviewed changes

example_dags/calculate_popular_movies.py Outdated Show resolved Hide resolved

feluelle reviewed Aug 18, 2022

View reviewed changes

kaxil reviewed Aug 18, 2022

View reviewed changes

jwitz reviewed Aug 18, 2022

View reviewed changes

kaxil added this to the 1.1.0 milestone Aug 18, 2022

tatiana reviewed Sep 5, 2022

View reviewed changes

utkarsharma2 added 3 commits September 7, 2022 02:26

Add the readme.md dag to example dags

b53a1fe

Move code from readme.md to integration tests

c4626bc

Add both curl and permalink to README.md file

815a607

utkarsharma2 force-pushed the RefCodeinMd branch from f2fac59 to 815a607 Compare September 6, 2022 21:04

tatiana reviewed Sep 6, 2022

View reviewed changes

minor change

4b9f393

tatiana reviewed Sep 6, 2022

View reviewed changes

utkarsharma2 added 3 commits September 7, 2022 02:45

minor change

5d79e43

minor change

9c0c99f

minor change

6274b25

tatiana approved these changes Sep 6, 2022

View reviewed changes

utkarsharma2 merged commit 62303bd into main Sep 6, 2022

utkarsharma2 deleted the RefCodeinMd branch September 6, 2022 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the readme.md example dag to example dags folder #681

Add the readme.md example dag to example dags folder #681

utkarsharma2 commented Aug 18, 2022

sunank200 left a comment

feluelle left a comment

utkarsharma2 commented Aug 18, 2022 •

edited

Loading

kaxil Aug 18, 2022 •

edited

Loading

jwitz Aug 18, 2022

tatiana Sep 5, 2022

jwitz Aug 18, 2022

tatiana Sep 5, 2022

feluelle Sep 6, 2022

utkarsharma2 Sep 6, 2022 •

edited

Loading

kaxil Sep 6, 2022

tatiana Sep 6, 2022

utkarsharma2 Sep 6, 2022

utkarsharma2 Sep 6, 2022 •

edited

Loading

utkarsharma2 Sep 6, 2022

feluelle Sep 7, 2022

tatiana Sep 6, 2022

tatiana Sep 6, 2022

utkarsharma2 Sep 6, 2022

codecov bot commented Sep 6, 2022

	https://github.com/astronomer/astro-sdk/blob/d5aa768b2d4bca72ef98f8d533fe3f99624b172f/example_dags/calculate_popular_movies.py#L1-L37
	curl -O https://raw.githubusercontent.com/astronomer/astro-sdk/main/example_dags/calculate_popular_movies.py

		@@ -50,42 +50,7 @@ pip install astro-sdk-python[amazon,google,snowflake,postgres]

		1. Copy the following DAG into a file named `calculate_popular_movies.py` and add it to the `dags` directory of your Airflow project:

	Alternatively you can download the file
	Alternatively, you can download the file by running:

Add the readme.md example dag to example dags folder #681

Add the readme.md example dag to example dags folder #681

Conversation

utkarsharma2 commented Aug 18, 2022

Description

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Checklist

sunank200 left a comment

Choose a reason for hiding this comment

feluelle left a comment

Choose a reason for hiding this comment

utkarsharma2 commented Aug 18, 2022 • edited Loading

kaxil Aug 18, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utkarsharma2 Sep 6, 2022 • 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

utkarsharma2 Sep 6, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 6, 2022

Codecov Report

utkarsharma2 commented Aug 18, 2022 •

edited

Loading

kaxil Aug 18, 2022 •

edited

Loading

utkarsharma2 Sep 6, 2022 •

edited

Loading

utkarsharma2 Sep 6, 2022 •

edited

Loading