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

Xlmod submodule #125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Xlmod submodule #125

wants to merge 2 commits into from

Conversation

mwalzer
Copy link

@mwalzer mwalzer commented May 11, 2022

Removed cv folder contents, replaced with a git submodule

@mwalzer
Copy link
Author

mwalzer commented May 11, 2022

submodule updates via

git submodule update --remote --recursive --merge
git push

@mwalzer mwalzer requested a review from edeutsch May 11, 2022 14:45
Copy link
Contributor

@edeutsch edeutsch left a comment

Choose a reason for hiding this comment

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

I don't understand how this works, but I hope it does! fine with me!

@edeutsch edeutsch requested a review from mobiusklein May 11, 2022 21:01
@mobiusklein
Copy link

Is the goal here is to make it so the previous URL https://github.com/HUPO-PSI/mzIdentML/blob/<commitish>/cv/XLMOD.obo resolvable, or to make it so git clone https://github.com/HUPO-PSI/mzIdentML still contains XLMOD.obo?

A submodule does not do the former since cv isn't a file as far as GitHub is concerned. A submodule could do the latter with the git clone --recurse-submodules. Submodules are imperfect solutions. I do not know if git subtree would work better. I don't remember which of the above goals we're trying to solve either.

@mwalzer
Copy link
Author

mwalzer commented May 16, 2022

Is the goal here is to make it so the previous URL https://github.com/HUPO-PSI/mzIdentML/blob/<commitish>/cv/XLMOD.obo resolvable, or to make it so git clone https://github.com/HUPO-PSI/mzIdentML still contains XLMOD.obo?

I think since XLMOD is now in a stand-alone project, it shouldn't resolve the XLMOD path for future commits in mzIdentML anymore.
And I also think it's okay that a default clone does not ship the CV. Developers should be aware of extra dependencies, and as such the submodule works for me and gives anyone the convenience to include XLMOD from the new sources with few extra steps.
But some documentation on this should be included in the PR - if we decide to go this route.

Submodules are imperfect solutions.

I tend to agree, but the alternative to leave an outdated 'hardcopy' in here I like even less. Maybe other alternatives exist, IDK how subtree would work.
@edeutsch Would removing it completely work, too?

@edeutsch
Copy link
Contributor

@edeutsch Would removing it completely work, too?

I don't really know.

If any code such as the validator relies on XLMOD.obo being in this specific place (in the old repo) I would imagine it would be very easy to adjust it to a new scheme. And maybe that is preferable to trying to get fancy with submodules.

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.

3 participants