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

push Base.push! and LibGit2.push! apart #24837

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Conversation

rfourquet
Copy link
Member

I just noticed that #24818 was incomplete while working on another PR: I know little about LibGit2, but its push! seems quite different from Base's. I also noticed 3 other functions for which the same serparation could be done, but I'm not sure, so I ask for feedback about them: count, map, split. LibGit2 may legitimately extend base functions in this case, the only "problem" is that it clutters the documentation at the REPL (not importing this module would solve this problem, but I guess it's not doable).

@rfourquet rfourquet added deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module labels Nov 29, 2017
@KristofferC
Copy link
Sponsor Member

KristofferC commented Nov 29, 2017

Do you know if moving LibGit2 to stdlib would solve this problem with documentation being lumped up together? I would guess no.

@rfourquet
Copy link
Member Author

Do you know if moving LibGit2 to stdlib would solve this problem with documentation being lumped up together? I would guess no.

I would guess no too.

This change forces to use a bunch of Base.push! from within the LibGit2 module, which is a bit ugly. As I said, I don't know this module, can someone confirm that LibGit2.push! has a different enough meaning than Base.push! to justify this split?

@rfourquet
Copy link
Member Author

Would this be considered as breaking if this is done in 1.x ?

@nalimilan
Copy link
Member

Because due to the name conflict all uses of LibGit2.push! would have to be qualified. But if we move LibGit2 to the stdlib, it should be fine (and we could have a deprecation for some time).

@rfourquet
Copy link
Member Author

Thanks! (my question was indeed poorly phrased, I meant to ask if it would be an acceptable breaking change, or if it would be moved to stdlib, to which you answered)

@JeffBezanson
Copy link
Sponsor Member

Let's rebase this and just try to get it in ASAP.

@rfourquet
Copy link
Member Author

Appveyor failure seems to be a timeout (2h job). This is ready to go, if someone wants to have a look?
As said in the OP, count, map and split could be considered for a similar treatment; I can open a PR if there is feed-back in favor.

@StefanKarpinski
Copy link
Sponsor Member

This we certainly should go ahead with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants