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

Update subpath display #5219

Merged
merged 3 commits into from
Sep 5, 2022
Merged

Update subpath display #5219

merged 3 commits into from
Sep 5, 2022

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Aug 3, 2022

Some examples:

-root-a_p is now pinned to file://${BASEDIR}/pinnes (a) (version dev)
+root-a_p is now subpath-pinned to directory /a in file://${BASEDIR}/pinnes (version dev)

-root-a-i_g is now pinned to git+file://${BASEDIR}/pinnes#master (a/i) (version dev)
+root-a_p is now subpath-pinned to directory /a in file://${BASEDIR}/pinnes (version dev)

-root-a_p.dev    rsync  file://${BASEDIR}/pinnes (a)
-root-b_g.dev    git    git+file://${BASEDIR}/pinnes#master (b)
+root-a_p.dev    rsync  directory /a in file://${BASEDIR}/pinnes
+root-b_g.dev    git    directory /b in git+file://${BASEDIR}/pinnes#master

--> retrieved root-a-i_g.dev  (git+file://${BASEDIR}/pinnes#master(a/i))
+-> retrieved root-a-i_g.dev  (directory /a/i in git+file://${BASEDIR}/pinnes#master)

-[root-a-i_g.l1] synchronised (git+file://${BASEDIR}/pinnes#master(a/i))
-[root-a-k_p.l1] synchronised (file://${BASEDIR}/pinnes(a/k))
+[root-a-i_g.l1] synchronised (directory /a/i in git+file://${BASEDIR}/pinnes#master)
+[root-a-k_p.l1] synchronised (directory /a/k in file://${BASEDIR}/pinnes)

@rjbou rjbou added this to the 2.2.0~alpha milestone Aug 3, 2022
@rjbou rjbou added the AREA: UI label Aug 3, 2022
@dra27
Copy link
Member

dra27 commented Aug 3, 2022

Could we even avoid the square brackets in the display?

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 3, 2022

We need to differentiate the main url and the subpath. We can use another separator.

@dra27
Copy link
Member

dra27 commented Aug 4, 2022

I was thinking we could kinda avoid it, but that would be inconsistent with opam pin add (i.e. unless we could make opam pin add git+https://github.com/dra27/package/subdir automatically infer the sub-directory, then I agree we need some kind of separator).

The problem is that the notation is opam specific, but perhaps we can therefore make it more explicit that there's a subpath pin involved. Possibility:

-root-a_p is now pinned to file://${BASEDIR}/pinnes (a) (version dev)
+root-a_p is now subpath-pinned to directory /a in file://${BASEDIR}/pinnes (version dev)

-root-a-i_g is now pinned to git+file://${BASEDIR}/pinnes#master (a/i) (version dev)
+root-a-i_g is now subpath-pinned to directory /a/i in git+file://${BASEDIR}/pinnes#master (version dev)

-root-a_p.dev    rsync  file://${BASEDIR}/pinnes (a)
-root-b_g.dev    git    git+file://${BASEDIR}/pinnes#master (b)
+root-a_p.dev    rsync  /a in file://${BASEDIR}/pinnes
+root-b_g.dev    git    /b in git+file://${BASEDIR}/pinnes#master


--> retrieved root-a-i_g.dev  (git+file://${BASEDIR}/pinnes#master(a/i))
+-> retrieved root-a-i_g.dev  (/a/i in git+file://${BASEDIR}/pinnes#master)

-[root-a-i_g.l1] synchronised (git+file://${BASEDIR}/pinnes#master(a/i))
-[root-a-k_p.l1] synchronised (file://${BASEDIR}/pinnes(a/k))
+[root-a-i_g.l1] synchronised (/a/i in git+file://${BASEDIR}/pinnes#master)
+[root-a-k_p.l1] synchronised (/a/k in file://${BASEDIR}/pinnes)

??

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 4, 2022

I like the idea, but i'm wondering if these two examples are explicit for user. I give it a quick try:

[root-a_p.l2] synchronised (/a in file://${BASEDIR}/pinnes)

-> do we understand that a is a subpath?

+Ok, root-b_g is no longer subpath-pinned to /b in git+file://${BASEDIR}/pinnes#master (version l2)

-> it is not clear if it is the subpath-pinning that is removed or the pinning

@dra27
Copy link
Member

dra27 commented Aug 4, 2022

Good point - perhaps:

[root-a_p.l2] synchronised (directory /a from file://${BASEDIR}/pinnes)

When un-pinning, perhaps there's no need for status at all - the fundamental change is that we've un-pinned that repo, so it could be the same message for both sub-path and normal?

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 4, 2022

When un-pinning, perhaps there's no need for status at all - the fundamental change is that we've un-pinned that repo, so it could be the same message for both sub-path and normal?

It is possible, it will complicate API as in different function, it's `OpamPinCommand.string_of_pinned" that is called for printing.

@rjbou
Copy link
Collaborator Author

rjbou commented Aug 4, 2022

It's a local function 👍

@rjbou rjbou force-pushed the subpath-display branch from 78bc810 to d8524dd Compare August 4, 2022 15:36
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

I like that subpath-pinned to directory /<path> in ... idea. LGTM

rjbou added 3 commits August 8, 2022 18:30
from 'git+https://url#hash (subpath)' to 'subpath in git+https://url#hash'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants