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

refactor: propagate version formatting errors #2566

Merged
merged 7 commits into from
Apr 10, 2021

Conversation

vladimyr
Copy link
Member

@vladimyr vladimyr commented Apr 9, 2021

Description

Propagate errors originating from VersionFormatter#format_version to the upper level.

Motivation and Context

Continuation of #2499
(It got closed before I managed to leave my review.)

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

/cc @offbyone

Copy link
Contributor

@offbyone offbyone left a comment

Choose a reason for hiding this comment

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

I'm mostly curious about the error handling rationale; I personally would prefer that version formatting be fairly silent, if possible, but that might be a "just me" philosophy. Otherwise, it looks like you reordered parameters in a few places, but ... ¯_(ツ)_/¯ different strokes for different folks :)


match formatted {
Ok(formatted) => Some(formatted),
Err(error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What conditions would lead to this warning? I would rather just silently format the raw version than log a warning when this happens.

Copy link
Member Author

@vladimyr vladimyr Apr 9, 2021

Choose a reason for hiding this comment

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

I would rather just silently format the raw version than log a warning when this happens.

This is silent unless you explicitly opt-in by setting STARSHIP_LOG environment variable to the warn level.

EDIT: warn is default log level so it gets print always but that is actually a good thing if you ask me

What conditions would lead to this warning?

The simplest case that would cause an error is a malformed version format like:

version = "v${raw" # missing right curly

This results in (with logging enabled):

[WARN] - (starship::modules::java): Error formating `java` version:
 --> 1:3
  |
1 | v${raw
  |   ^---
  |
  = expected variable_name

@vladimyr
Copy link
Member Author

vladimyr commented Apr 9, 2021

I'm mostly curious about the error handling rationale; I personally would prefer that version formatting be fairly silent, if possible, but that might be a "just me" philosophy.

As I explained above #2566 (comment) this is still silent. Error handling is moved to the upper level because version formatter doesn't know who called it thus can't print contextually rich warnings.

Otherwise, it looks like you reordered parameters in a few places, but ... ¯_(ツ)_/¯ different strokes for different folks :)

This speaks for itself ie I find the original version so good I didn't feel a need to change things around 😉
I did reordering to achieve the following form: format_version(version /*[1]*/, format /*[2]*/) where version is followed by format so it reads like: version formatter formats version [1] using provided format [2].

@vladimyr
Copy link
Member Author

vladimyr commented Apr 9, 2021

Otherwise, it looks like you reordered parameters in a few places

Actually, there is more than just that. I did 3 additional things there:

  1. ensured that every format_{module}_version function has following signature format_{module}_version(version, version_string) (no context, config, just format string)
  2. simplified version parsing in a few of those
  3. renamed a bunch of java tests from test_parsing_* to test_format_* in order to follow already established convention

@vladimyr vladimyr requested a review from a team April 9, 2021 01:35
src/formatter/version.rs Outdated Show resolved Hide resolved
@andytom andytom merged commit fe030c2 into starship:master Apr 10, 2021
@vladimyr vladimyr deleted the refactor-version-formatter branch April 10, 2021 17:13
DavidSmith166 pushed a commit to DavidSmith166/starship that referenced this pull request Apr 24, 2021
* refactor: propagate version formatting errors

* refactor: trim version formatting boilerplate

* refactor(node): unwrap version formatting

* docs: fix typo

* docs: remove dots after `version_format`

* feat: lazy version parsing

* refactor(version-formatter): collect segments into string
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.

4 participants