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

Use cli in messages #1928

Closed
wants to merge 26 commits into from
Closed

Use cli in messages #1928

wants to merge 26 commits into from

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Dec 6, 2023

fix #956

This is a rough draft. I would understand you'd rather take this on yourself, but in case it could be of any use, I thought I'd share!

Edit: CI won't cooperate. but anyway. I will try this branch of usethis installed locally and will update if something goes wrong.

Edit2 : feel free to take anything that you like from this!

List of problems

Nice to have?

ui_code that is meant to be run and other that is not meant?

Results

Impact of changes in be8a0f0

image

not covered by tests

  • git_sitrep(): may be a problem as it uses `ui_todo with multiple lines input extensively, and is not really tested)
  • pr_push()

@jennybc
Copy link
Member

jennybc commented Dec 7, 2023

Thanks for taking this on! In terms of me planning when to turn my attention to this and review it, do you consider it semi-done now? As in, you're at a stopping place and the PR is in a good state for review and discussion?

@olivroy
Copy link
Contributor Author

olivroy commented Dec 7, 2023

I consider it to be semi-done in the sense that I may only apply touchups!

The biggest issue is how glue / crayon handles Linebreaks and whitespace differs.

I'd say my biggest concern is that I modified the ui_*() functions thinking only of usethis UI, and ignoring revdeps, which is why my PR may not be the correct way forward.
I also haven't edited any documentation, but will do this after review!

I will continue to play with it, and until you review, the only remaining commits would just be to patch how usethis messages look / feel, as I use usethis from this branch more!

For the reiview, I would look at the test changes, the changes of the ui_*() functions. (mostly whitespace / linebreaks changes or issues). I don't know what is your best practice for testing usethis, but for me, it is just to use it in the wild, so you are welcome to try usethis from this branch. pak::pak("olivroy/usethis@cli") to inform your review!

I could not figure out how to make the bullet for ui_todo() red, nor the info yellow for ui_info(), (not mentioned in the cli guide either. Maybe ui_todo could use the right arrow instead of a red dot?

@olivroy olivroy marked this pull request as ready for review December 12, 2023 14:25
@olivroy
Copy link
Contributor Author

olivroy commented Dec 12, 2023

@jennybc Feel free to take a look! I personally encountered no issues since my last commit

@olivroy olivroy mentioned this pull request Jan 17, 2024
@olivroy

This comment was marked as resolved.

@jennybc
Copy link
Member

jennybc commented Feb 13, 2024

I should let you know I now have my own branch where I'm doing the cli conversion with a pretty different approach. So you might want to pause this effort 😅

@olivroy
Copy link
Contributor Author

olivroy commented Feb 14, 2024

Looking forward to see it! Let me know if you want some feedback!

@jennybc
Copy link
Member

jennybc commented Mar 1, 2024

#1956 has finally landed! I'd welcome comments from you, but you should not feel obligated if you don't have the time or interest. But given all the work you did here (I know how much it is!), I thought you might want to take a look. I will merge #1956 fairly soon, because I think it's clearly a huge step forward and the real proof will be in having some folks use the dev version and kick the tires.

@olivroy olivroy closed this Mar 1, 2024
@olivroy olivroy mentioned this pull request Mar 14, 2024
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.

Migrate ui_*() functionality to use cli
2 participants