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

[#226] Change error message formatting #307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

int-index
Copy link
Member

@int-index int-index commented Nov 11, 2024

Description

Problem: multiple issues with the CLI output

  1. In the --help output, metavars GLOB_PATTERN and REPOSITORY_TYPE used spaces instead of underscores. Variables typically don't contain spaces in their names, so it looked confusing.
  2. Incorrect or unintuitive coloring of the output. For example, filenames were dim even though they're important and should stand out, the report of success wasn't green, error messages weren't red, etc.
  3. Excessive indentation and empty lines that made it difficult to visually parse the error report. Also the use of the exotic character that looks bad in many font configurations.
  4. Filenames and line numbers were far apart, making it impossible to Ctrl+Click to jump to the source of the error.
  5. In case of connection failure, the output was too verbose and platform-dependent.

Solution:

  1. Include the filename in positions returned by the scanner.
  2. Adjust the pretty-printing functions and update test output.
  3. Extend VerifyError with a constructor for connection failures.

Related issue(s)

Fixes #226.

✅ Checklist

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:
  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

✓ Release Checklist

  • I updated the version number in package.yaml.
  • I updated the changelog and moved everything
    under the "Unreleased" section to a new section for this release version.
  • (After merging) I edited the auto-release.
    • Change the tag and title using the format vX.Y.Z.
    • Write a summary of all user-facing changes.
    • Deselect the "This is a pre-release" checkbox at the bottom.
  • (After merging) I updated xrefcheck-action.
  • (After merging) I uploaded the package to hackage.

@int-index int-index marked this pull request as draft November 11, 2024 00:38
@serokell-bot
Copy link
Contributor

serokell-bot commented Nov 11, 2024

1 Warning
⚠️ Please use <nickname>/<issue-id>-<brief-description> format for branch names.
Example: lazyman/#123-my-commit

Generated by 🚫 Danger

@int-index int-index force-pushed the int-index/formatting branch 4 times, most recently from 4c2306b to be889c2 Compare November 21, 2024 23:55
@int-index int-index changed the title Draft: Change error message formatting [#226] Change error message formatting Nov 22, 2024
@int-index int-index force-pushed the int-index/formatting branch from be889c2 to 7189218 Compare November 23, 2024 03:19
@int-index int-index marked this pull request as ready for review November 23, 2024 03:19
Copy link
Member

@gromakovsky gromakovsky left a comment

Choose a reason for hiding this comment

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

Some changes look a bit subjective, but I can't say I'm strongly against anything, so let me approve. @Martoon-00 may review this week, let's give him a chance as well.

@Martoon-00
Copy link
Member

Looks good, I see why these changes are reasonable and agree.

One concern: after merging error message for every single reference into one block, discrepancies in indentation became more apparent:

  • The difference in leading - being present at one line and being absent at another. Maybe we can remove this - in lists after all. (Clearly the last line is not a list, but the difference still stands out)
check-images.md:20:1-34: bad reference:
  The reference to "bad image ref 3" failed verification.
  - link (relative): ./3.png      <------
  File does not exist:
    ./3.png                               <------
  • Indentation size here:
The reference to "Reference lowered" failed verification.
  - anchor (file-local): uppercase-name
  Anchor 'uppercase-name' is not present, did you mean:
      - UPPERCASE-NAME (handmade) 14:3-27

Maybe these are worth changing to look uniform. WDYT?

@int-index
Copy link
Member Author

Agreed, I'll try to change the phrasing further to avoid this issue.

@int-index int-index force-pushed the int-index/formatting branch 2 times, most recently from 0934d6e to 3d5eab1 Compare December 22, 2024 17:15
@int-index
Copy link
Member Author

I've pushed a change, @Martoon-00 would you be so kind to take another look?

This time indentation should be consistent. Also I've modified the scanner so that all positions include the filename.

@int-index int-index force-pushed the int-index/formatting branch 3 times, most recently from 69f6e3f to 5dab3a9 Compare December 22, 2024 23:19
Problem: multiple issues with the CLI output

1. In the --help output, metavars GLOB_PATTERN and REPOSITORY_TYPE
   used spaces instead of underscores. Variables typically don't
   contain spaces in their names, so it looked confusing.
2. Incorrect or unintuitive coloring of the output. For example,
   filenames were dim even though they're important and should
   stand out, the report of success wasn't green, error messages
   weren't red, etc.
3. Excessive indentation and empty lines that made it difficult to
   visually parse the error report. Also the use of the exotic '➥'
   character that looks bad in many font configurations.
4. Filenames and line numbers were far apart, making it impossible
   to Ctrl+Click to jump to the source of the error.
5. In case of connection failure, the output was too verbose and
   platform-dependent.

Solution:

1. Include the filename in positions returned by the scanner.
2. Adjust the pretty-printing functions and update test output.
3. Extend VerifyError with a constructor for connection failures.
@int-index int-index force-pushed the int-index/formatting branch from 5dab3a9 to 5dd0139 Compare December 23, 2024 00:20
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.

Revise styling of the output
4 participants