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

Add Feature: Printing Absolute Path in Context #122

Merged
merged 6 commits into from
Jun 11, 2022
Merged

Conversation

HelinXu
Copy link
Contributor

@HelinXu HelinXu commented May 9, 2022

ic.configureOutput(includeContext=True, absPath=True)

absPath, False by default, can be used to control whether ic()'s output includes the absolute path of the file where ic() is called, provided includeContext == True. This is useful when ic() is called from multiple files in a project, with files potentially having same names but different paths. Moreover, for most editors such as VSCode, a clickable link to the line where ic() is called is provided. E.g. a click on /absolute/path/to/example.py:12 brings you to the line of the debugging call. It is really handy when you have debugging outputs from multiple files and want to easily jump to the line where the debugging call is made.

icecream/icecream.py Outdated Show resolved Hide resolved
@alexmojaki
Copy link
Collaborator

Anyway, this LGTM. @gruns any general thoughts?

@gruns
Copy link
Owner

gruns commented May 17, 2022

@HelinXu please rename the absPath parameter to contextAbsPath so it's clear what path, exactly, is being controlled via the absolute path parameter. beyond that, lgtm 🙌

@alexmojaki zooming out, i think replacing the includeContext param, prefix param, and soon-to-be contextAbsPath param, of configureOutput() with one, single function that lets users customize all the content before the ic output, ie replaces both the prefix and context, could improve output flexibility. that way users can fully customize ic's output to their exact liking, albeit in a manner that exposes ic's internals like the callFrame and callNode currently used in IceCreamDebugger._formatContext()

we could then provide a pre-built function that replicates the current behavior. something like

from icecream import ic, createPrefixFn

_formatPrefix = createPrefixFn(prefix='ic |', includeContext=True, contextAbsPath=True)
ic.prefixFn  = _formatPrefix

conversely we could also just improve the documentation so people feel its easier to subclass IceCreamDebugger() and extend _formatContext() to their own liking and wait for more people to express a desire to customize the prefix and/or context

thoughts?

@HelinXu
Copy link
Contributor Author

HelinXu commented May 17, 2022

Done. Thanks for the advice 🙌

@gruns
Copy link
Owner

gruns commented May 18, 2022

awesome. thank you, @HelinXu!

last step is to add appropriate tests for contextAbsPath in https://github.com/gruns/icecream/blob/master/tests/test_icecream.py

see https://github.com/gruns/icecream/blob/master/tests/test_icecream.py#L440-L468 for how includeContext is tested, along with https://github.com/gruns/icecream/blob/master/tests/test_icecream.py#L108-L140 which tests lines for the inclusion of context

with tests, ill merge your amazing work here! 🙌

@HelinXu
Copy link
Contributor Author

HelinXu commented May 18, 2022

Sure, I'm working on that.

One more thing, there're actually two slightly different handlings when it comes to absolute path, i.e. abspath() and realpath(). They are basically dealing with symbolic links differently:

def abspath(path):
    """Return an absolute path."""
    path = os.fspath(path)
    if not isabs(path):
        if isinstance(path, bytes):
            cwd = os.getcwdb()
        else:
            cwd = os.getcwd()
        path = join(cwd, path)
    return normpath(path)


# Return a canonical path (i.e. the absolute location of a file on the
# filesystem).

def realpath(filename):
    """Return the canonical path of the specified filename, eliminating any
symbolic links encountered in the path."""
    filename = os.fspath(filename)
    path, ok = _joinrealpath(filename[:0], filename, {})
    return abspath(path)

I'm using realpath() for now🤔. Any suggestions on this?

@HelinXu
Copy link
Contributor Author

HelinXu commented May 18, 2022

tests added :)

test_icecream.py::TestIceCream::testContextAbsPathMultiLine PASSED                                 [ 19%]
# Output with absolute path can easily exceed line width, so no assert line num here.
test_icecream.py::TestIceCream::testContextAbsPathSingleLine PASSED                                [ 22%]

I've written lineIsAbsPathContext(line), similar to lineIsContext(line). It seems "with/without abs path output" doubles test cases for several config options, but I did not bother to create tests for all of them as it's too redundant.

tests/test_icecream.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@HelinXu HelinXu left a comment

Choose a reason for hiding this comment

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

I've resolved the unattended TODO and have modified MYFILEPATH according to the latest commit. Sorry for the carelessness XD

@gruns gruns merged commit dc880a5 into gruns:master Jun 11, 2022
@gruns
Copy link
Owner

gruns commented Jun 11, 2022

merged! 🎉

thank you for your wonderful suggestion and, most importantly, your wonderful PR, @HelinXu 🙌

@HelinXu
Copy link
Contributor Author

HelinXu commented Oct 11, 2022 via email

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.

3 participants