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.

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

DOC: create shared includes for comparison docs #38771

Closed
wants to merge 2 commits into from

Conversation

afeld
Copy link
Member

@afeld afeld commented Dec 29, 2020

This will help ensure consistency between the examples.

Similar to #38735, but focusing on shared content in a different folder. Particularly interested to see if the build fails in the same way.

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

This will help ensure consistency between the examples.
@@ -65,24 +65,9 @@ Filtering in SQL is done via a WHERE clause.

SELECT *
FROM tips
WHERE time = 'Dinner'
LIMIT 5;
Copy link
Member Author

@afeld afeld Dec 29, 2020

Choose a reason for hiding this comment

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

This is the only change of substance: left out the LIMIT and the .head(5) below since they aren't needed and LIMIT is covered elsewhere.

@afeld afeld changed the title DOC: create shared includes for content shared by comparison docs DOC: create shared includes for comparison docs Dec 29, 2020
@afeld
Copy link
Member Author

afeld commented Dec 29, 2020

Heh, this pull request is failing differently than #38735: flake8-rst is giving F821 undefined name 'tips'. I tried the :flake8-add-ignore: F821 Custom Role, but that didn't work.

Sigh. Though I planned to use these includes for the Excel page, maybe not worth the trouble.

Thoughts?

@gfyoung gfyoung added the Docs label Dec 29, 2020
@jreback jreback added this to the 1.3 milestone Dec 29, 2020
:ref:`boolean indexing <indexing.boolean>`

.. ipython:: python

Copy link
Contributor

Choose a reason for hiding this comment

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

i think in all of these you need a :suppress: directive that defines tips :->

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. It's a bit of a hack, so let me know if you'd like it done differently.

:suppress:

# ensure tips is defined when scanning with flake8-rst
if 'tips' not in vars():
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you can just add this

   url = (
       "https://raw.github.com/pandas-dev"
       "/pandas/master/pandas/tests/io/data/csv/tips.csv"
   )
   tips = pd.read_csv(url)

but maybe @jorisvandenbossche has a better soln

Copy link
Member

Choose a reason for hiding this comment

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

But we would want to avoid to read this data multiple times in each file during the doc build, I think, so ideally we can let flake-rst ignore this in a different way

Copy link
Contributor

Choose a reason for hiding this comment

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

But we would want to avoid to read this data multiple times in each file during the doc build, I think, so ideally we can let flake-rst ignore this in a different way

sure, any idea how?

@jorisvandenbossche
Copy link
Member

Heh, this pull request is failing differently than #38735: flake8-rst is giving F821 undefined name 'tips'. I tried the :flake8-add-ignore: F821 Custom Role, but that didn't work.

Do you have the commit that tried this? Did you add it to the list of extensions in conf.py?

@afeld
Copy link
Member Author

afeld commented Dec 30, 2020

Pushed it up in #38837.

@afeld
Copy link
Member Author

afeld commented Jan 3, 2021

Closing in favor of #38887.

@afeld afeld closed this Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants