Skip to content

Conversation

@tschm
Copy link
Contributor

@tschm tschm commented Nov 15, 2025

No description provided.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Not sure what this is doing except adding >>> in front of code?

I do not see why we should do that.

Plus, you seem to be reformatting code so some lines do not fit on the screen anymore.
Maybe also let's not do this?

Plus my usual comments:

  • please write a descriptive summary of your PR, I have a hard time understanding what you want to do here. Use AI if you need to, some description is better than none.
  • where possible, do different things in separate PR, e.g., adding output coercion to portfolio_performance is not "test readme"

@tschm
Copy link
Contributor Author

tschm commented Nov 15, 2025

No, try to provoke the test with faulty code in README.md

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 15, 2025

?? I am sorry, I genuinely have no idea what you are referring to.

Can you please add a description to the PR and explain its purpose?

There should be no >>> at the start of the code in the readme.

@tschm
Copy link
Contributor Author

tschm commented Nov 16, 2025

Please ignore for now. Please read up on doctest

@tschm tschm marked this pull request as draft November 16, 2025 02:25
@fkiraly
Copy link
Collaborator

fkiraly commented Nov 16, 2025

@tschm, I know how doctest works and have written doctest plugins for pytest myself.

Please don't assume lack of knowledge but provide an explanation or description what you are intending to do here.

Regarding doctest: the >>> are used to indicate code in a docstring. The docstring is then displayed without the >>>. Now you inserted >>> in the readme, which gets displayed including the >>>, so turns perfectly fine code into code with syntax error once users want to copy it.

@tschm
Copy link
Contributor Author

tschm commented Nov 16, 2025

I am not 100% sure on those >>>. I use them to indicate a doctest executable python command where as rows without them indicate the expected output (tested sign for sign). Yes, they make copy some inconvenient, but better than faulty code in a README...

@tschm tschm marked this pull request as ready for review November 16, 2025 13:42
@fkiraly
Copy link
Collaborator

fkiraly commented Nov 18, 2025

Yes, they make copy some inconvenient

The entire point of the readme is to provide the most conveniently possible entry point to potential users. So I would strongly oppose any change that reduces user convenience substantially for a little bit of developer convenience.

Speaking about developer convenience, it is easy to get the python blocks. Just grep anything between triple-backtick-python and triple-backtick lines.
No need for a complicated abuse of doctest.

@fkiraly fkiraly changed the title Test readme [MNT] testing code in README for correctness Nov 18, 2025
@fkiraly
Copy link
Collaborator

fkiraly commented Nov 18, 2025

do I understand correctly, you want to test code in the README for correctness? (that it runs)

That would be a very useful CI step!

Though I would do it differently, and that avoids doing the business with the >>>:

  • grep code blocks as the lines between triplebacktick-python and triplebacktick
  • execute them in the test env

Done

@tschm
Copy link
Contributor Author

tschm commented Nov 19, 2025

Yes, I want to test the code in the README. So the >>> have an advantage

>>> 2+2
4

A few comments about this block. The "4" is the expected result. Replacing "4" with "5" should break the test.
Also note the empty line below 4 and in front of the close triplebacktick. If you remove it the triplebacktick becomes part of the expectation! Of course, if we remove the 4 and the >>> we test whether 2+2 runs. That's better than no test but not as powerful as testing for expected output

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