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

rewrite the library: simpler design and implementation, unit tests #26

Merged
merged 19 commits into from
Oct 20, 2023

Conversation

amtoine
Copy link
Owner

@amtoine amtoine commented Oct 9, 2023

ok, this is quite a big one but bear with me, i'll explain everything 😉

changelog

  • remove docs/ => out of date
  • remove toolkit/ => out of date
  • remove sugar completions git => let's use carapace or use the ones from nu_scripts
  • remove sugar dotfiles => a bit out of scope
  • i left the rest of the sugar module but moved it to nu-git-manager/sugar/ => i'd like to refactor the namespace of this as well in a follow-up PR

Note
i've written command names, arguments, type annotations and tests for internal tool commands with the goal of being self-contained in mind without requiring lengthy documentation 🤞

  • the nu-git-manager git private module is about Git related tool commands
    • git url focuses on manipulating Git URLs
  • the nu-git-manager fs private module is about interacting with the filesystem
    • fs store focuses on interacting with the local store of repositories
  • tests cover git url parse-git-url and git url get-fetch-push-urls with nupm test
  • not sure how to properly test fs store without interacting with the host environment 🤔
  • the git and fs modules are basically simplified and encapsulated versions of the previous utils utils module
  • gm itself
    • i've removed the ghq API => it was making the code too complex and honestly not sure we care about that kind of compatibility
    • not that gm is not a module anymore, but a true command with subcommands, i.e. you don't use nu-git-manager gm but use nu-git-manager * or use nu-git-manager ["gm list", "gm clone"]
    • i think the easiest here is to just read the docstrings and the arguments of the exported commands
    • now one of the biggest changes, and the one that gave me the motivation to do the rewrite, was that, when cloning a repo with the SSH protocol, you end up with the origin remote setup with SSH for both FETCH and PUSH. with that, even if the repo is public and you only git fetch, you still have to write the passphrase for the SSH key, which is a bit of a pain... 😆 now, as you can see in the examples of gm clone, one can use the --ssh, --fetch and --push options to tweak that in the clone step 👌
      there's even autocompletion for --fetch and --push 🥳
  • the README has been updated => have a read
  • finally i bump the version to 0.2.0

@amtoine amtoine requested a review from melMass October 9, 2023 17:32
@amtoine amtoine self-assigned this Oct 9, 2023
@amtoine amtoine removed the request for review from melMass October 9, 2023 17:32
@amtoine
Copy link
Owner Author

amtoine commented Oct 9, 2023

@melMass
i'll ping you when it's ready 😉

@amtoine amtoine marked this pull request as ready for review October 10, 2023 16:30
@amtoine amtoine requested a review from melMass October 10, 2023 16:30
@amtoine
Copy link
Owner Author

amtoine commented Oct 10, 2023

i found a few bugs during this refactor, i propose to solve them after it lands 😊

  • bare repos are not listed by gm list
  • gm list depends on GNU find but i have a better alternative
  • gm clone fails with both --bare and --remote

@amtoine
Copy link
Owner Author

amtoine commented Oct 10, 2023

i'll also update the README of Nupm itself 👍

@amtoine amtoine changed the title refactor rewrite the library: simpler design and implementation, unit tests Oct 10, 2023
amtoine pushed a commit to amtoine/dotfiles that referenced this pull request Oct 10, 2023
amtoine pushed a commit to amtoine/dotfiles that referenced this pull request Oct 11, 2023
@melMass
Copy link
Collaborator

melMass commented Oct 19, 2023

I just learned that Github support nushell syntax!! 😮

manage your Git repositories with the main command of `nu-git-manager`

Usage:
  > gm

Subcommands:
  gm clone - clone a remote Git repository into your local store
  gm list - list all the local repositories in your local store
  gm remove - remove one of the repositories from your local store
  gm root - get the root of the local store of repositories managed by `nu-git-manager`

Flags:
  -h, --help - Display the help message for this command

Input/output types:
  ╭───┬─────────┬─────────╮
   # │  input  │ output  │
  ├───┼─────────┼─────────┤
   0  nothing  nothing 
  ╰───┴─────────┴─────────╯

@melMass
Copy link
Collaborator

melMass commented Oct 19, 2023

Great work, I agree with all the directions on simplification.
It all looks good to me, I'm just going to fully test it on windows and mac as I already see a few non cross platform commands.
I asked on discord but asking here too just in case: If I want to add some shared utils by the modules, not exposed by the lib what would you recommend?

this should bring the new CI from main and run the tests.
@amtoine
Copy link
Owner Author

amtoine commented Oct 19, 2023

I just learned that Github support nushell syntax!! 😮

pretty cool huh 😉

@amtoine
Copy link
Owner Author

amtoine commented Oct 19, 2023

Great work, I agree with all the directions on simplification. It all looks good to me, I'm just going to fully test it on windows and mac as I already see a few non cross platform commands. I asked on discord but asking here too just in case: If I want to add some shared utils by the modules, not exposed by the lib what would you recommend?

very happy you like the directions 🙏

as said on Discord, let's add stuff to modules like fs/ or git/ for private commands 👍

if we can fix the CI on Windows, that's perfect 💪

@amtoine
Copy link
Owner Author

amtoine commented Oct 20, 2023

let's go with this 🥳

thanks for the review @melMass 🙏

@amtoine amtoine merged commit d46bedb into main Oct 20, 2023
3 checks passed
@amtoine amtoine deleted the refactor branch October 20, 2023 13:06
amtoine added a commit to amtoine/dotfiles that referenced this pull request Oct 20, 2023
This was referenced Oct 28, 2023
amtoine added a commit that referenced this pull request Nov 1, 2023
this removes the `[toc]` that have been introduced in the TOC of the
README in #26
@amtoine amtoine added the breaking-change Changes that will break the API or behaviour label Nov 5, 2023
amtoine added a commit that referenced this pull request Dec 3, 2023
should
- close #106 
 
# description
as `nu-git-manager-sugar gist` hasn't been touched since #26, it's old,
unmaintained, possibly broken and does not fit the _philosophy_ of NGM,
e.g. it does not export `gm ...` commands 🤔

this PR removes the `gist` module from `sugar` for now, until someone
can reimplement it and make sure it's functional 👍
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that will break the API or behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants