-
Notifications
You must be signed in to change notification settings - Fork 49
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: Hardlink to index.html #722
Conversation
None of my questions are answered. The repo does miss contributor documentation for things like this. |
It is not directly used by pydoctor: it’s designed like this to keep compatibility with old links that refers to the root module name. So all modules can be accessed by their full name. Regarding the contributors documentation.. please keep in mind that we’re all volunteers here. Best documentation will always be to read the code. For the branch model: ne cherches pas midi à 14:00 like we say French: just target the master branch for your PRs. |
I am a dev and a maintainer, too. But beeing a volunteer is IMHO not good excuse for not documenting. Code is not documentation. And contributors documentation does not have something to do with code by the way. It is more about how to project is organized and how to do PRs, how the test/build/CI system is intended to work etc pp. But lets stop the meta discussion and lets go back to this PR. How can I help you to fix this PR and get it merged. |
First, your changes makes pydoctor crash. The cause of this must still be investigated. Secondly, there is a bug in pydoctor_primer that crashes. I’m not sure whether it’s because the CI step is only working for privileged users or because the diff to show is too big because the change is making pydoctor crash and the diff is then too large.
We need to examine the stack trace and fix the bug. It might work on your environment because your already have a html build existing so your not seeing the crash. Please try to delete the output directory and see if pydoctor still works. |
I don't see a crash. Can you link to the output in some of that Microsoft GitHub Actions CI jobs/machines?
I do not know what that is and it is not documented somewhere and not even in the primar yaml itself.
In that case I can reproduce this problem.
Again: I can not see this error in your CI or tests. How can an extern but motivated contributor can see this? You should document this. Your project is not ready for extern contributors because PRs and even testing is possible only with project-intern knowledge. You can blame me by the way: Your test suite do catch the problem very well. I just haven't run the tests on my local machine.
About the testsI remember why I didn't run the tests. You use the real filesystem instead of a virtual filesystem for your tests (see #721). About About |
x = list(root_module_path.parent.glob('*')) | ||
print(f'TTTT\n{root_module_path=} {x=}') | ||
print(f'{root_module_path.exists()=}') | ||
os.link( |
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.
OK, I need your help here. First of all I think I confounded the arguments in os.link()
. So I changed them but this does not solve the problem.
The value of root_module_path
is PosixPath('apidocs/basic.html')
which does not exist. That seems to be the problem. But I do not understand why it does not exist or even if it have to exists.
Of course because of that the os.link()
can not work.
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.
The value of root_module_path is PosixPath('apidocs/basic.html') which does not exist. That seems to be the problem. But I do not understand why it does not exist or even if it have to exists.
I believe it doesn’t exist because it hasn’t been generated yet at the time the index page is generated. This is because the summary pages are written before the individual files, you can see that here:
Line 131 in 662eafa
writer.writeIndividualFiles(subjects) |
I guess a symlink doesn’t need the file to exists while a hard link need it to exists. So the fix would be to call writeIndividualFiles
before.
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.
Make totally sense. Thanks for analyzing. Because this is not your own code, are you sure that your solution will do the job? Or I might should do some more research if there is a better location in code to call writeIndividualFiles()?
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 sure: I haven’t tried it.
https://github.com/twisted/pydoctor/actions/runs/7346693679/job/20001879420?pr=722
The most important is the unit tests, you can disregard the pydoctor_primer failure at this time.
I started contributing to pydoctor in 2020 when they had no documentation whatsoever. Our contributor documentation talk about using tox for testing, it doesn’t talk about the primer because it’s something that I just set up. You can look at this PR to understand the goal of the primer, that is to comment PRs and show the diff in the pydoctor log: #739 (comment) I will investigate this problem when I have some time next week, in the mean time I’ll try to answer your questions |
The solution is not that simple. I tried
Other tests then fail. (btw: I miss the opportunity to run a single test only. Not sure how to do this with tox. Pytest alone still do not work.) I am not enough into the code to understand it and to decide about a solution. My suggest would be refactor In the current state my understanding of the code and the logic behind it is not deep enough to find a solution myself. The test summary
It seems that the original |
A hardlink is created instead of a symlink to
index.html
. Also added hypothesis entry to.gitignore
.I had to use
os
module becausepathlib.Path.hardlink_to()
is introduced in Python 3.10 butpydoctor
to support Python 3.6.It works in my productive environment.
Two tests (via pytest) are failing and I'm not sure why. They are well documented but I lack of deeper understanding of how pydoctor works to understand the problem.
I didn't use tox. Maybe you could update your contrib info about the purpose of your tox setup.
Btw: Where in the HTML output is that hardlink HTML file used? My package "buhtzology" do have a
buhtzology.html
hardlink toindex.html
. But nowhere in the HTML output I can find a link to abuhtzology.html
file.Your contrib info (btw: missing a TOC) didn't tell about branches or branching model. So I branched from and into
master
. Hope this is OK that way.Fix #720