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

show basename in source: split line #1017

Merged
merged 2 commits into from Nov 28, 2023
Merged

Conversation

bartman
Copy link
Contributor

@bartman bartman commented Nov 22, 2023

This patch modifies the text shown above the source window as part of the context_source() function.

I'm working on a complex C++ project, with many directories and files.
Before this change I would get a split with...
────────────────────────── source:/server/externa[...].cpp+130 ────
which does not tell me anything about what file I am in, other than its extension.

After the change, I get something like this:
─────────────── source:/server/externa[...]SendReceive.hpp+110 ────

Depending on the length of the path the following may be shown:

  • if len(fn) <= 30
    • show the full path
  • elif len(basename(fn)) < 20
    • show <first 15 chars>[...]<basename>
  • else
    • show <first 15 chars>[...]<last 10 chars>

before this change I would get a split with...
`────────────────────────── source:/server/externa[...].cpp+130 ────`
which does not tell me anything about what file I am in.

now I get something like this...
`─────────────── source:/server/externa[...]SendReceive.hpp+110 ────`
Copy link

🤖 Coverage Update

  • Commit: 5927df4
  • Current Coverage: 71.6387%
  • New Coverage: 71.6387%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

@Grazfather
Copy link
Collaborator

Looks good, but why the switch from 20 chars to 30? I wish we had comments about where that came from in the first place.

Looks like the old code could have made a long line if the extension was long as well.

Overall this lgtm.

@bartman
Copy link
Contributor Author

bartman commented Nov 27, 2023

Looks good, but why the switch from 20 chars to 30?

I don't see myself ever debugging in a terminal that's less than 80 columns, and I would prefer to see more of the file name if it's long. I could switch it back to 20 if it's a problem.

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Minor change, otherwise LGTM

gef.py Outdated Show resolved Hide resolved
Copy link

🤖 Coverage Update

  • Commit: 5927df4
  • Current Coverage: 71.6387%
  • New Coverage: 71.6387%
  • Diff: 0.0

To this point, this PR:

  • does not include changes to tests
  • does not include changes to documentation
  • does not include forbidden words

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

LGTM

@bartman
Copy link
Contributor Author

bartman commented Nov 28, 2023

LGTM

thank you for your review.

the CI is failing with

I think both are setup issues, and not related to my change.

Is there anything I need to do to finish this PR?

@hugsy
Copy link
Owner

hugsy commented Nov 28, 2023

Yup I saw the failures, it's not on your end. I'll be looking into that
Merging

@hugsy hugsy merged commit 788f56b into hugsy:main Nov 28, 2023
4 of 6 checks passed
@hugsy hugsy added this to the 2024.01 milestone Dec 22, 2023
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.

None yet

3 participants