-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fixes #160: adding support to generate documentation from multiple repository in WEBUI #169
Conversation
backend/generator.js
Outdated
} | ||
}); | ||
return true; | ||
// logic for one Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sch00lb0y I know this is still a WIP
, but I have a concern here. This is not a good design. The webUI shouldn't handle build logic. the purpose of the webUI should be to just take a list of subproject url's and pass them to generate.sh
. In the event if there is no subproject pass a empty string. Handle the whole build logic in generate.sh
. That way this would also work with travis out of the box. This is why some more discussion was needed in #160 before starting work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the build process are happening in the bash script only, I just added a condition for a single repository or multiple repositories. We don't know how many features will be implemented in future. when we keep on adding thing to a single file, we making things much complex and also making hard to read as well as maintain. still we don't know building documentation for multiple repositories is our end goal. even i'm reusing old scripts like toc tree generation. we'll maintain separate script for WEBUI and travis. as discussed @imujjwal96 will work on Travis implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me correct you there, @pri22296 is working on Travis implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying add everything to generate.sh. You could create a different bash script and source it. That's not a problem. I don't understand why is the condition even needed? You could just have checked in the bash script whether SUBPROJECT_URLS is empty or not and if not empty clone them too. One of our primary goals should be that output remains same regardless of if the user used travis env vars
, .yaydoc.yml
or webUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What i did when i was working on implementing multiple projects' script was, I added an if statement in the generate_ci.sh
that checked if the SUBPROJECT_URLS is set or not. If it was set, it executed another script, otherwise it executed generate.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pri22296 I have to disagree with you on this one. We cannot merge a PR on UI changes unless the backend script is working. So removing the script part from this PR would keep this PR on hold for a very long time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pri22296 Yeah sure I'll restrict this PR with UI. . If you already wrote the script. please make a PR I'll integrate with WEBUI. @imujjwal96 I just wrote only a few lines in build scripts and i'll scrape this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, how does this PR fixes partly #151 . I don't see any python test files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry wrongly mentioned 😅 . I'll change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need @Sch00lb0y , that's already scrapped.
c1322b1
to
4bcab9f
Compare
@imujjwal96 @pri22296 Please review |
@Sch00lb0y could you add a screenshot or maybe show a deployment? |
@pri22296 demo deployment http://54.255.170.238/ |
@Sch00lb0y Here also you are using the first entry of the sub-projects as the giturl. don't do that. have a seperate field for giturl like it was before. and then have a seperate field for subprojects which would initially contain nothing and if users want they would add them. notice the word sub. The structure here is a tree. don't flatten it. |
@pri22296 I'm not using the first URL of subproject as git_url. I'm taking as two entry as you mentioned in the previous PR #174. I added a button which add input box for the subproject and I'm joining with ',', passing it to script. var email = formData.email;
var gitUrl = formData.gitUrl;
var docTheme = formData.docTheme;
var debug = formData.debug;
var uniqueId = uuidV4();
var webUI = "true";
var subProject = ""
if (formData.subProject != undefined) {
subProject = formData.subProject.join(",")
}
var donePercent = 0;
const args = [
"-g", gitUrl,
"-t", docTheme,
"-m", email,
"-d", debug,
"-u", uniqueId,
"-w", webUI,
"-s", subProject
]; even i didn't touch the script. I'm just passing value to the script. if this is not the answer for your query. please elobrate your query |
@pri22296 I have changed the lable please check |
github api search is not working for subprojects as it works for giturl. |
@pri22296 I made changes |
Looks good. the initial suggestion when field is empty is still from the giturl. can that be fixed? |
@pri22296 sorry, I'm not able to get you Please elaborate your query. |
@pri22296 I made this intentionally because it should wait for github api. if the same search query. It'll improve the experience |
@imujjwal96 @pri22296 please review this |
Ok, it makes sense for the giturl field but Why would giturl and one of it's subproject will have the same query? |
|
Oh cool. nice job |
@@ -90,7 +97,21 @@ function getData() { | |||
if (field.name === "debug" ) { data.debug = field.value; } | |||
if (field.name === "heroku_api_key" ) { data.herokuAPIKey = field.value.trim(); } | |||
if (field.name === "heroku_app_name" ) { data.herokuAppName = field.value.trim(); } | |||
if (field.name === "subproject_url[]") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is []
required in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To represent array I kept like this. if you want me to change then I'll change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a standard notation for representing arrays? If it is, then it's fine, otherwise remove it. []
in a field name looks weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it is common to use
Allow empty values for sub project's url |
Failed to generate documentation logs:
|
@imujjwal96 weird. coz i tried generating it and got a fine result |
You have the input that I provided and the logs that it generated. |
I tried it with other repos. With your input, I'm also getting a blank page. Currently there are many issues with multiple_generate.sh. I will be trying to sort all of them in my PR for #196 . In the time being, maybe just review this PR for UI only? |
@imujjwal96 because yaydoc having index.rst in the docs folder which is causing the problem. This is get solved as soon as #199 get merged |
@Sch00lb0y I will have to disagree with you on this one. The example with which i tried also had index.rst in the docs folder. |
@pri22296 I think none of the repository index.rst is linking the file which is outside the docs folder. because in multiple_generate.sh we deleting everything other than docs folder |
made changes for this |
public/scripts/form.js
Outdated
@@ -29,14 +30,20 @@ $(function () { | |||
|
|||
socket.on('logs', function (data) { | |||
$('#messages').append($('<li class="info">').text(data.data)); | |||
$("#progress").css("width", data.donePercent + "%"); | |||
if (data.donePercent >= 100) { | |||
$("#progress").css("width", "90%"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this mean that if progress goes from 95 to 100, the bar would actually roll back to 90?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it won't go between 90 to 100.some time multiple_generate.sh will execute some time generate.sh will execute. we don't know how many logs will emit in future so, I made a tap that I should not go above 90 before execution of exit code 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be (data.donePercent >= 90)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made changes @imujjwal96 Thanks
…tiple repository in WEBUI
Description
I have added button to add the multiple input box for handling multiple repository
Related Issue
Motivation and Context
If project have more than one repository. it'll help to combine all the repository documentation under one umbrella
How Has This Been Tested?
Screenshots (if appropriate): generated documentation
Types of changes
Checklist:
Fixes #<number> commit message