Skip to content

Adding utils and tests for hello.py#261

Merged
monchier merged 12 commits intostreamlit:developfrom
monchier:207
Oct 8, 2019
Merged

Adding utils and tests for hello.py#261
monchier merged 12 commits intostreamlit:developfrom
monchier:207

Conversation

@monchier
Copy link
Copy Markdown
Contributor

@monchier monchier commented Oct 3, 2019

The purpose of this PR is adding a test for some utility in hello.py. I created a package for hello.py, modified cli.py and I added a util file to be used by hello.py. I then added a test for such util file.

@monchier monchier requested review from jrhone and tvst October 3, 2019 22:49
@monchier monchier added the WIP label Oct 3, 2019
@monchier monchier removed the WIP label Oct 4, 2019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Capitalize comments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is a variable name in this case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this needed? We could just have cli.py call run(), right?

Copy link
Copy Markdown
Contributor Author

@monchier monchier Oct 4, 2019

Choose a reason for hiding this comment

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

Yeah,... well... hello works by grabbing the source file path of hello.py. Not sure it will be ok by just "calling it". Is it just because of re-run? I understand we won't be modifying the source code at runtime in this case, but the API may break.


    filename = streamlit.hello.__file__

    # For Python 2 when Streamlit is actually installed (make install rather
    # than make develop).
    if filename.endswith(".pyc"):
        filename = "%s.py" % filename[:-4]

    _main_run(filename)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did ended up removing this and refactoring the imports.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can get a function's docstring with the_func.__doc__. Wouldn't it be easier / safer to just use that here?

For example:

doc = the_func.__doc__.splitlines()
last_line = len(doc)
return lines[last_line + 1:]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can try this. Seems a good option. Not sure on top of my head if the_func.__doc__.splitlines() has to maintain the same formatting and gives the same number of lines, but it may as well be.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not tell me about the lines with """, it only gives me the lines with the actual content of the docstring. So,

"""
"""

and

"""  """

are the same. I would still have to parse and do something with these, so not sure of the actual advantage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to write it and it quickly became as complex. I would leave it as it is. I am not a big fan of ad hoc logic like this one...

Copy link
Copy Markdown
Contributor Author

@monchier monchier Oct 4, 2019

Choose a reason for hiding this comment

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

Honestly there might not be need for doing all this introspection... we could just remove the docstring and factor out the comments, then require all demo functions not to have a docstring (easier assert). The current code happened because of some initial back and forth between me and Adrian, but no reason to stay this way. If you like this approach better I can refactor in this sense, it will take an extra hour or so. ... Evil of introspection...

@monchier
Copy link
Copy Markdown
Contributor Author

monchier commented Oct 4, 2019

I removed the remove_docstring function and removed the use of introspection to get the description of the functions via docstring. Now, the description is passed as an item in the map that describes the demos (DEMOS in hello.py)

I also moved the actual demo code out to a separate file demos.py.

@monchier monchier merged commit b12dd46 into streamlit:develop Oct 8, 2019
@monchier monchier deleted the 207 branch October 8, 2019 18:30
tconkling added a commit to tconkling/streamlit that referenced this pull request Oct 9, 2019
* develop:
  Fullscreen button to expand tables and charts (streamlit#137)
  Blacklist for hashing (streamlit#254)
  Adding utils and tests for hello.py (streamlit#261)
  Fix port-finding bugs (streamlit#280)
  Fix hello.py usage of os.path.join on a URL (streamlit#285)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants