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

Docstring update for the indegree, outdegree and degree functions in core.jl #419

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

damianodegaspari
Copy link

Fixed the documentation of the functions indegree, outdegree and degree and removed the TODO comment in the source code that was asking for it. I am new to GitHub, Julia and programming in general. Please check carefully and do not hesitate to let me know of any mistake.

Fixed the documentation of the functions indegree, outdegree and degree and removed the TODO comment in the source code that was asking for it.
@damianodegaspari damianodegaspari changed the title Update core.jl Docstring update for the indegree, outdegree and degree functions in core.jl Feb 28, 2025
@damianodegaspari
Copy link
Author

I just received an email telling me about those failing tests. I do not know what those are. What should I do so that they pass?

@henrik-wolf
Copy link
Contributor

The tests fail due to the formatting of the code. You should either run the julia formatter on the changed file (in vscode use "format document with...") or explicitly run it on the files (see JuliaFormatter quick start) I would guess that it complains about the double empty line after add_vertices! (line 47)

@damianodegaspari
Copy link
Author

damianodegaspari commented Mar 11, 2025

I see. I admit I had missed the Contributor Guide page of the package. I followed the link you gave to JuliaFormatter. Running it on the file using format("file.jl") did not give visible changes (in particular the additional empty line you pointed out did non disappear). In the contributor guide page I linked above BluStyle is mentioned. So I did format("file.jl", BlueStyle()) and this time the additional empty line disappeared. However, it also seems to have added spaces in front of the names of all functions. I hope the tests pass. Maybe I should have first asked if that was ok, and only then commit. As I already wrote, I am new to all of this, so please do not hesitate to point out bad behaviour.

@henrik-wolf
Copy link
Contributor

I am surprised that it replaced the spaces with tabs... do you have indentation with tabs set up in vscode? Did you run the testsuite locally on your machine? (see the testing workflow in the docs)
The blue style says that indentation should be done with 4 spaces, rather than tabs. If you can find a way to revert the indentations, and see the test pass locally, we can get someone to approve the workflow of this request.

@henrik-wolf
Copy link
Contributor

To add a review style comment though, as this has been a lot of talk about formalities: I think your changes of the docstrings are well written and do help clarify the behaviour of these functions. As such, I am in favour of merging this PR, once we figured out the formatting.

@damianodegaspari
Copy link
Author

damianodegaspari commented Mar 16, 2025

Thanks for helping and for the positive feedback. As far as I can tell my VSCode has always been set up with spaces and never with tabs (on the bottom right it says Spaces: 4. I identified the failing test as the line @test format(Graphs; verbose=false, overwrite=false). So I did format("src/core.jl", verbose=false). Now the test just mentioned passes. I did not rerun all tests because on my laptop it takes quite some time, but given our discussion there should not be other problems. The first of the two commits with the same title is just one of my experiments, it should be ignored: I just failed not to include it as a commit. What would the next step be now?

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.

2 participants