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.

Sign up for GitHub

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

Jump to bottom

Implement Docsrv #124

Merged
merged 2 commits into from
Oct 16, 2017
Merged

Implement Docsrv #124

merged 2 commits into from
Oct 16, 2017

Conversation

dpordomingo
Copy link
Contributor

No description provided.

@dpordomingo
Implement docsrv
ca5f3d9 
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
@codecov
Copy link

codecov bot commented Oct 16, 2017 •
edited
Loading

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   83.72%   83.72%           
=======================================
  Files          17       17           
  Lines         983      983           
=======================================
  Hits          823      823           
  Misses         98       98           
  Partials       62       62

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 c73f347...54de51a. Read the comment docs.

@abeaumont abeaumont self-requested a review October 16, 2017 09:51
@abeaumont abeaumont self-assigned this Oct 16, 2017
abeaumont
abeaumont suggested changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@abeaumont abeaumont left a comment

Choose a reason for hiding this comment

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

I think docsrv should be transparent for repositories, instead of forcing them to follow specific (undocumented?) rules and maintain specific build rules. I guess that's not possible for some reason.

Makefile
-include $(MAKEFILE)

# Docsrv: configure the languages whose api-doc can be auto generated
LANGUAGES = go
Copy link
Contributor

Choose a reason for hiding this comment

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

There are also java bindings, should it be reflected here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Java api was not defined as a requirement; it could be added as a feature request.

Makefile Outdated
# Docsrv: configure the languages whose api-doc can be auto generated
LANGUAGES = go
# Docsrv: configure the directory containing the python sources
PYTHON_MAIN_DIR ?= ./python
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no python dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a default value, used if there is python code (that do not interfere if there is no python code)
Anyway, I can delete it from enry for sure (I'll do so in a new PR)

README.md
@@ -120,7 +120,7 @@ Note that even if enry's CLI is compatible with linguist's, its main point is th
Java bindings
------------

Generated Java binidings using a C shared library + JNI are located under [`java`](java)
Generated Java binidings using a C shared library + JNI are located under [`java`](https://github.com/src-d/enry/blob/master/java)
Copy link
Contributor

Choose a reason for hiding this comment

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

docsrv cannot infer relative references? Shouldn't it be better to add support to infer them instead of forcing every repo to avoid them? (which will be hard)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be proposed as a feature request, but for this first release docsrv only converts md to html instead of parsing/replacing links

@dpordomingo
Copy link
Contributor Author

@abeaumont docsrv would serve a small group of repos in its first version. For that group, I'd PR the needed changes, and after that, I'll document the final interface. We decided to proceed that way to try covering a representative group of projects before deciding a final interface.
Regarding to the necessity of adding a make rule for implementing docsrv, it was defined as an initial requirement. In the future, there will be more info in the repository that will be used for docsrv, like logos, descriptions...

@dpordomingo
Cleanup
54de51a 
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
@dpordomingo
Copy link
Contributor Author

@abeaumont answered your doubts inline.
Added a commit 54de51a with your python suggestion.

@abeaumont abeaumont merged commit 1c163c5 into src-d:master Oct 16, 2017
@dpordomingo dpordomingo mentioned this pull request Oct 17, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abeaumont abeaumont abeaumont approved these changes

Assignees

@abeaumont abeaumont

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpordomingo @abeaumont