-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add __str__
method to classes
#213
Conversation
a415791
to
fbf61e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice @MarkWieczorek! I found a typo on the semimajor axis longitude (and I added a degree symbol to specify the units).
I also added some suggestions to make the outputs of the __str__
to look more similar to the format proposed by @leouieda in #213. Let me know what do you think!
I don't think you should worry about docstrings for this method. I don't think the __str__
method needs a description, so it should be fine to leave it without docstring.
Some of the lines in __str__
are not being tested. We could add some tests for them, but I'm ok if we want to leave them untested (we can still visually verify that the method works as expected).
On a side note, I like the idea of having a dataclass for each reference (as @VascoSch92 suggested), so we could still add the references in the __str__
but without the need to add the full text: we could only return the author's name, year and doi. Alternatively, we could have either a reference as a string or as a list of strings, so we can improve how they are printed. But I think we could work on that after we merge this PR.
fix typo and add degree symbol Co-authored-by: Santiago Soler <santisoler@fastmail.com>
Here is some progress. First, I didn't implement the "left justified bullet + centered text" as I think it make things look more complex than it needs to be. I'll do whatever people think is best, and this PR isn't quite done yet anyway. There's still room for improvement. Second, the above changes try to fix the multiple reference issues. I added line breaks in the reference string (which is probably better anyway), and then textwrapped each reference separately. I think this looks fine when multiple references are present, but you could argue that maybe we don't need the hanging indent when only one is present. Here's what it looks like:
I need to think about the code coverage issue. I think that the issue is that the docstring doesn't test "Comments" and a few don't test "References". Perhaps its best just to add these declarations to the doc string. |
I changed slightly the docstring tests by (1) making the reference shorter, and (2) adding a comment. Here is an example:
For sphere, I used the same data as in bl.Moon2015. However, code-cov (though better) is still placing a warning at the lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MarkWieczorek!
Here is some progress. First, I didn't implement the "left justified bullet + centered text" as I think it make things look more complex than it needs to be. I'll do whatever people think is best, and this PR isn't quite done yet anyway. There's still room for improvement.
Sure, no worries! We can keep it simple then. I would add the bullets so it's clear we are listing the attributes that define the ellipsoid. The bullet character I used is an ASCII character, so it'll show fine on any terminal. What do you think?
Second, the above changes try to fix the multiple reference issues. I added line breaks in the reference string (which is probably better anyway), and then textwrapped each reference separately. I think this looks fine when multiple references are present, but you could argue that maybe we don't need the hanging indent when only one is present. Here's what it looks like:
I love the hanging indent. Adding the line break is a nice and simple solution. I'd like to retake the references at some point (different PR), to think if we should use a dataclass or maybe allow to take a collection of references. But for this PR I think the new line character is perfect. I'll just leave one minor comment regarding this (see below).
However, code-cov (though better) is still placing a warning at the lines if self.reference is not None: and if self.comments is not None:, even though these get used in the doctest. Not sure what to do about that....
Codecov is putting a yellow exclamation mark on those lines:
This doesn't mean that those lines are not tested (they are actually being tested in the doctest). But codecov is highlighting them because we don't have any tests that check what happens when the if statement is not True (when comments
is None
). Basically, it complains because we are only testing one possible scenario. For this particular PR, I think it's ok not to reach 100% coverage for this: we'll still have visual inspection of this method in the documentation pages.
Here's with bullets, and with the changes to the line breaks
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'm merging it. Thanks @MarkWieczorek for the work!
Squash and Merge summary
Add a
__str__
method to theSphere
,Ellipsoid
andTriaxialEllipsoid
classes that displays information about the realization in more readable way than__repr__
. Use a new line character to separate mnultiple references in the realizations. Polish a few of the examples in the docstrings.This PR adds the
__str__
method to the three Boule classes. When usingprint
, the output is formatted asNote that I placed the method after all of the properties. I also did not include a dock string. I'm not sure what the best practice is with this method.
Relevant issues/PRs:
Closes #181