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

[fix][vendoring] Rename simple-linguist to enry #38

Merged
merged 1 commit into from
Jun 26, 2017
Merged

[fix][vendoring] Rename simple-linguist to enry #38

merged 1 commit into from
Jun 26, 2017

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Jun 26, 2017

Simple rename of simple-linguist to enry in the Glide files and one source file. This actually fix the build of the server for me (previously it gave an error about an "internal reference" to a module), which should not happen since in theory they're the same tags but happens anyway.

@juanjux juanjux changed the title [Vendoring] Rename simple-linguist to enry [fix][Vendoring] Rename simple-linguist to enry Jun 26, 2017
@juanjux juanjux changed the title [fix][Vendoring] Rename simple-linguist to enry [fix][vendoring] Rename simple-linguist to enry Jun 26, 2017
@codecov
Copy link

codecov bot commented Jun 26, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@9df972e). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master     #38   +/-   ##
========================================
  Coverage          ?   60.2%           
========================================
  Files             ?      13           
  Lines             ?     691           
  Branches          ?       0           
========================================
  Hits              ?     416           
  Misses            ?     235           
  Partials          ?      40
Impacted Files Coverage Δ
language.go 77.77% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df972e...d785cbe. Read the comment docs.

@@ -223,7 +223,7 @@ imports:
- transport
- name: gopkg.in/src-d/go-errors.v0
version: 0c303ec4c027302259ec1738f19f515aad25f13f
- name: gopkg.in/src-d/simple-linguist.v1
- name: gopkg.in/src-d/enry.v1

Choose a reason for hiding this comment

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

Stupid question: Shouldn't we use github.com/src-d/enry instead now that we are versioning using commits?

Now that we are using glide, what we want is to use a particular commit from github.com/src-d/enry, not from gopkg.in/src-d/enry.v1, right?

Choose a reason for hiding this comment

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

@alcortesm We cannot do this because all the internal enry imports are using gopkg.in/src-d/enry.v1 .

Choose a reason for hiding this comment

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

@ajnavarro that's right. But our internal imports use gopkg.in/src-d/enry.v1 because we were not using glide before and we needed some way to import a particular version. Now that we are using glide, that should no longer be the case.

What I am talking about is to start importing github.com/src-d/enry everywhere, now that thanks to glide, we no longer need to use URL redirects to simulate versioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, at this point I don't know what we should or shouldn't be doing with respect to versioning, is there any document or meeting minute detailing this?

Copy link
Member

Choose a reason for hiding this comment

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

We have to use the sale import that the library defines. Enry does use gopkg.in in it's importa, so we do it too. Glide cannot change that.

Choose a reason for hiding this comment

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

👍

Copy link
Member

@smola smola left a comment

Choose a reason for hiding this comment

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

Do not use an alias for the package, use it's name.

@smola
Copy link
Member

smola commented Jun 26, 2017

@juanjux I've just merged another PR renaming the package, please, rebase.

@juanjux
Copy link
Contributor Author

juanjux commented Jun 26, 2017

@smola actually, something is probably odd in enry because I tried that but if I just import the module without any aliasing and then use enry as the module, the travis build fails with:

./language.go:4: imported and not used: "github.com/bblfsh/server/vendor/gopkg.in/src-d/enry.v1" as slinguist
./language.go:11: undefined: enry in enry.GetLanguage
./language.go:13: undefined: enry in enry.OtherLanguage

I'll try importing and aliasing as enry.

@juanjux
Copy link
Contributor Author

juanjux commented Jun 26, 2017

@smola actually did that but without adding the enry alias it searched for the slinguist symbol (wtf) and travis failed, was going to try with the enry alias but I've seen just merged PR #39 just did that so I'm rebasing (the other files in this still need to be modified).

@erizocosmico
Copy link
Contributor

@juanjux yep, forgot to update the glide stuff.

@smola smola merged commit d88ef53 into bblfsh:master Jun 26, 2017
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.

5 participants