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

feat: add pijul_channel module #4765

Merged
merged 14 commits into from Dec 31, 2022
Merged

Conversation

IslandUsurper
Copy link
Contributor

@IslandUsurper IslandUsurper commented Dec 27, 2022

Description

Adds a module to detect Pijul repos and display the channel name.

Motivation and Context

When a project uses Pijul as its VCS, it is nice to have some indication of it the way Git and Mercurial do. This module is basically a copy of the hg_branch module. It has the same settings and functionality, except there is no concept of bookmarks in Pijul as there is in Mercurial, so it gets only.

Screenshots (if appropriate):

Screenshot

How Has This Been Tested?

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

As a VCS integration module, the tests are marked as #[ignore] so that developers don't have to have Pijul installed to run tests. In addition to automatic unit tests, I have installed Starship modified with this module and switched channels on a Pijul repository, and the channel name was reflected on the command line.

Checklist:

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

@IslandUsurper
Copy link
Contributor Author

It seems there are problems with building pijul from source, so I'll stop burning GH actions on this for now.

@IslandUsurper
Copy link
Contributor Author

Well, I don't have a Windows machine to test on, and I just learned about vcpkg last night, so I've probably done as much as I can here. I copied the workflow actions for Windows from https://github.com/boringcactus/pijul-windows-builds/ (linked from the official Pijul docs, despite being unofficial), so they should work, but I probably missed something and can't see it.

src/modules/pijul.rs Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
@roland-5
Copy link

It seems there are problems with building pijul from source, so I'll stop burning GH actions on this for now.

Do cargo install pijul --version 1.0.0-beta.2 --locked.

@IslandUsurper
Copy link
Contributor Author

IslandUsurper commented Dec 28, 2022

I did use --locked in 5301faf. 😉

Installing from source is too expensive, and compiled binaries are only
available for Windows (and unofficially as well). Perhaps once Pijul
1.0.0 comes out of beta there will be more binaries available in package
  repos.
| `symbol` | `' '` | The symbol used before the pijul channel name of the repo in your current directory. |
| `style` | `'bold purple'` | The style for the module. |
| `format` | `'on [$symbol$channel]($style) '` | The format for the module. |
| `truncation_length` | `2^63 - 1` | Truncates the pijul channel name to `N` graphemes |
Copy link
Member

Choose a reason for hiding this comment

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

To be more compatible with 32-bit systems:

Suggested change
| `truncation_length` | `2^63 - 1` | Truncates the pijul channel name to `N` graphemes |
| `truncation_length` | `2^32 - 1` | Truncates the pijul channel name to `N` graphemes |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both git_branch and hg_branch take i64 for truncation_length. While I agree that this is an easy change for 32-bit compatibility, I think consistency is more important. All of the modules that use truncation need to be using usize instead of i64.

Copy link
Member

Choose a reason for hiding this comment

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

Either way, I feel like an unsigned integer type should be preferred and meson also uses u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but let's do a follow-up change to handle all of the affected modules, instead of just polishing this one.

Copy link
Member

Choose a reason for hiding this comment

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

I've avoided doing this, because it could be a breaking change. If for instance they are switched to u32 the former default would be outside the accepted value range, and negative values would lead to errors instead of warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the risk of causing XKCD 1172, I doubt anything would actually break. Defaults shouldn't be stored in any TOML files, and anyone who actually sets truncation_length for any module is most likely to set it less than 200. As for handling negative values, I wonder if there's a way to adjust the deserializer to emit warnings with defaults instead of hard errors. Possibly we could make the default 0 instead of INT_MAX, as at these sizes, there's no practical difference.

Copy link
Member

Choose a reason for hiding this comment

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

I think some people will have based their config-file on starship print-config --default, it was even recommended in the docs for a few weeks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof. That does make a mess of things then.

pub symbol: &'a str,
pub style: &'a str,
pub format: &'a str,
pub truncation_length: i64,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub truncation_length: i64,
pub truncation_length: u32,

symbol: " ",
style: "bold purple",
format: "on [$symbol$channel]($style) ",
truncation_length: std::i64::MAX,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
truncation_length: std::i64::MAX,
truncation_length: u32::MAX,

src/modules/mod.rs Outdated Show resolved Hide resolved
@davidkna davidkna changed the title feat: Pijul module feat: add pijul_channel module Dec 29, 2022
@andytom andytom merged commit 67b6376 into starship:master Dec 31, 2022
@andytom
Copy link
Member

andytom commented Dec 31, 2022

Thank you for your contribution @IslandUsurper and thanks for reviewing @davidkna.

Indyandie pushed a commit to Indyandie/starship that referenced this pull request Jul 26, 2023
* feat: Pijul VCS support

* Extra bits needed for new module.

* Format Markdown table.

* Fix lint.

* Don't test Pijul module so thoroughly.

Installing from source is too expensive, and compiled binaries are only
available for Windows (and unofficially as well). Perhaps once Pijul
1.0.0 comes out of beta there will be more binaries available in package
  repos.

* Format!

* Bad rebase, remove Pijul install from workflow.

* Mock Pijul commands for code coverage.

* Make fake .pijul directory in fixture.

* Truly mock `pijul` command.

* Rename module from `pijul` to `pijul_channel`.

* Format!

* Fix config-schema.json.

* Missed changing module name in docs/ folder.
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

4 participants