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.

Sign up for GitHub

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

Jump to bottom

Control printing #45

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Control printing #45

wants to merge 11 commits into from

Conversation

paul019
Copy link
Collaborator

@paul019 paul019 commented Apr 12, 2024 •
edited
Loading

Fixes #34

Splines
Splines requested changes Apr 12, 2024
View reviewed changes
src/api/printable_result.py Outdated Show resolved Hide resolved
src/api/res.py Outdated Show resolved Hide resolved
src/api/res.py Show resolved Hide resolved
@Splines
Copy link
Collaborator

Splines commented Apr 12, 2024

Note that if #41 is merged beforehand, we will have a small merge conflict in wiz.res() here, but that shouldn't be a big issue.

@paul019 paul019 requested a review from Splines April 21, 2024 21:22
@Splines
Copy link
Collaborator

Splines commented Aug 14, 2024

Ok, so this looks good to me now! As this is modifying api/res.py, we just have to merge #41 beforehand and then resolve the conflicts here.

paul019 and others added 4 commits September 9, 2024 11:18
@paul019
Squashed commit of the following:
1f775ff 
commit e8d3467
Author: Splines <37160523+Splines@users.noreply.github.com>
Date:   Fri Sep 6 19:02:36 2024 +0200

    Implement new `wiz.res()` API (#41)

    * Remove plum as dependency

    * Implement new `wiz.res()` API (see #33)

    * Fix errors in error messages

    * Use consistent comment format in error messages

    * Add docstrings to `wiz.res()`

    * Add missing `int` type for uncertainties

    * Add PrintableResult to res() method signature

    * Rename `uncert` to `uncerts` (plural)

commit cc6bf5b
Author: paul019 <39464035+paul019@users.noreply.github.com>
Date:   Sat Apr 27 20:25:54 2024 +0200

    Improve wording: use `uncertainty` instead of `error` (#43)

    * Remove spaces in units

    * Detect percent as unit in console stringifier

    * Replace error by uncert

    * Undo unwanted changes

    * Replace error by uncertainty in various places

commit 62c34a5
Author: Splines <37160523+Splines@users.noreply.github.com>
Date:   Sat Apr 27 19:45:34 2024 +0200

    Add missing `site.baseurl` to relative links (#52)

commit f5e0007
Author: Splines <37160523+Splines@users.noreply.github.com>
Date:   Sat Apr 27 19:45:19 2024 +0200

    Truncate Readme & refer to documentation (#51)

    * Truncate Readme & refer to documentation

    * Remove GitHub-flavored markdown

    Otherwise, it won't be rendered correctly on PyPI.

    * Make Readme image clickable (points to docs)

commit 7af242c
Author: Splines <37160523+Splines@users.noreply.github.com>
Date:   Wed Apr 24 17:56:11 2024 +0200

    Add first version of docs (#37)

    * Init _Just the docs_ basic structure

    * Add Jupyter-related items to TODO

    * Add GitHub pages docs workflow

    * Add 404 page

    * Add landing page of documentation

    * Add LaTeX rendered output

    * Add quickstart & backbone for other pages

    * Add `export()` & siunitx troubleshooting

    * Add docs for `wiz.res()`

    * Add `wiz.res()` tip for multiple occurrences

    * Add config options & siunitx tips

    * Improve Jupyter section & add "Is this really for me?"

    * Add about page

    * Add table of contents (TOC) back to about page

    * Add docs build workflow for PRs

    * Specify correct cwd for docs workflow

    * Fix remaining docs workflow errors

    * Add tip for how to suppress output in JupyterNotebook.

    This fixes #44.

commit 63f2a6b
Author: Splines <37160523+Splines@users.noreply.github.com>
Date:   Mon Apr 22 23:31:50 2024 +0200

    Update GitHub URLs to point to new repo location (#47)
@paul019
Disable pylint warning
2ef0ce1
@Splines
Revert "Squashed commit of the following:"
336a71d 
This reverts commit 1f775ff.
@Splines
Merge branch 'main' into feauture/control-printing
9cfeaa4
Splines
Splines requested changes Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@Splines Splines left a comment

Choose a reason for hiding this comment

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

Please don't squash commits from other branches as this messes with Git (e.g. on GitHub we suddenly saw 32 files changes instead of just 2). You probably wanted to use a normal merge commit for that. I've just reverted your changes and then merged main into this branch.

But indeed use squash commits when merging a feature branch into main, as we don't want all individual commits there.

src/api/res.py
@@ -33,6 +33,7 @@ def res(
stat: Union[float, int, str, Decimal, None] = None,
sigfigs: Union[int, None] = None,
decimal_places: Union[int, None] = None,
print: Union[bool, None] = None, # pylint: disable=redefined-builtin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of just disabling this error message, shouldn't we rename print to something different to avoid overriding the internal print method accidentally? E.g. use should_print.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Splines Splines Splines requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature idea: with automatic printing, also allow to *not* print one result
2 participants
@paul019 @Splines