-
Notifications
You must be signed in to change notification settings - Fork 36
Contributing guidelines #251
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
Conversation
I think the triggering of the registration bot can only be done by people of the organization though |
Yes, AFAIK @theogf is right, this can only be done by us. So I think it is not really relevant for general or one-time contributors. More generally, like many Julia packages KernelFunctions.jl uses the ColPrac contributor guide and links to it in the README. |
If we want to add a specific CONTRIBUTING.md file, maybe we could just link to ColPrac? E.g., we could follow the suggestion in ColPrac and add
But since it does not contain much information, I am not sure it is helpful? |
I completely missed that badge amongst all the others at the top of the README. I think even a mostly-empty contributing.md file with appropriate links would be valuable. |
It also saves you from having to explain steps/workflows/processes to new contributors again and again :) so if you're doing anything else that you think others might benefit from knowing, think of where it can be added so it's visible. |
1. Merge the bumped version number into `master` (see above); normally this will be the commit of a squash-merged pull request. | ||
2. Go to the Github URL for this merge commit and write a comment saying `@JuliaRegistrator register`. | ||
|
||
The next release will then be processed automatically. KernelFunctions.jl does use TagBot so you can ignore the message about tagging. Note that it may take around 20 minutes until the actual release appears and you can edit the release notes if needed. |
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 just want to clarify (doesn't have to be added here, I think) that the Github releases and tags are nice but not relevant or mandatory for Julia packages (there exist many packages that work just fine without TagBot or Github releases). As soon as the PR by JuliaRegistrator is merged into the general registry, users can update KernelFunctions (well, in principle at least - sometimes it takes a while until the Julia PkgServers update the registry...).
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.
If not here, should this be clarified elsewhere? Again, this is about how would the process look like for a new contributor?
I am still not sure if the information about version numbers and JuliaRegistrator are helpful and important for new contributors. Maybe it would be sufficient to mention ColPrac (which also includes information about version numbers) and to explain how the code has to be formatted? For instance, I assume it would be good to mention that one should run
before opening a PR. And maybe these things would be more visible in a pull request template than a separate CONTRIBUTING.md file? |
Here's one data point: I was missing this information and would have liked to see it. Yes, adding how to format is one of those (which I hadn't thought of because I wasn't new to it, having been the one to introduce it 😉)! And maybe another line for how to run the tests. I'd actually find it really helpful to have a small Makefile so I can just type Adding it to a pull request template would make it even more visible, but clutter every new PR. Though how about adding a single line to the PR template pointing to CONTRIBUTING.md? |
I can understand the motivation but I think we should not add a I guess this is also why in general I am a bit sceptical regarding the current draft of CONTRIBUTING.md - SemVer and also JuliaRegistrator, TagBot, CompatHelper etc. are standards in the Julia package ecosystem that are used in almost every other Julia package. Thus this information is really not KernelFunctions-specific and widely known. I.e., I think it's only useful for very, very few people which we could just point to ColPrac, SemVer, and all the other resources equally well if needed 🙂 Thus probably it's also not very useful to add this information to every PR... |
It is widely known amongst people who already know it. How welcoming do you want to be to people who may come from the domain (kernels / GPs / probabilistic modelling) but may be new to Julia? If you would welcome their contributions: what can make it easier for them to get on-boarded ?
How else would you prefer to point people to those resources ? What else would you like to change about this draft before it gets merged? |
I agree with @devmotion that this is not relevant to contributors unless they have writing rights on the repo. |
About the contributing guidelines, it would be great to add a few explanation about our testing systems:
|
I'd be keen to pick this up again. I've been struggling personally with figuring out how to actually run the Julia development workflow. It would be great if packages like those of JuliaGaussianProcesses could convince more researchers to join the development of Julia-based code by making it easy for people to discover a package and get started. This could be as simple as some pointers to further reading. For everyone who's been involved in the package development for a longer time this may seem trivial, but it can be a significant - and unnecessary - barrier to entry for people who might be familiar with the maths, even good t coding, but new to the Julia ecosystem. (And just googling for "julia package development workflow" doesnt) So what can we do to make it easier for people to pick up new tools and contribute back to them? Adding to @theogf's points, here are some more common use-cases that I think would be good to document:
|
There's a separate VSCode plugin for JuliaFormatter: https://github.com/domluna/JuliaFormatter.jl#editor-plugins |
In my experience it is terrible... I just end up calling JuliaFormatter myself. |
I think it works pretty well but maybe something changed in recent versions. IIRC it also uses special commandline options that reduce latency. |
@st-- what's the status of this PR? Is there anything that you need help with in order to get it merged? |
Co-authored-by: willtebbutt <wct23@cam.ac.uk>
Co-authored-by: willtebbutt <wct23@cam.ac.uk>
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 think there are problems with the Makefile. Additionally, do we actually want to recommend its use? make test
doesn't seem much shorter than running ] test
in a Julia REPL started with julia --project=.
(or using the functionality available in VSCode and other editors).
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
As a new user I'd find it really valuable to have an easy way to run the "canonical" version of a command. |
I would like to improve the workflow, I guess I'm not sure about the Makefile mostly because KernelFunctions would be the only Julia package that I know that would include and suggest to use a Makefile. I don't know any tools in the ecosystem that are available for this workflow and I am worried that it might be more confusing than helpful for contributors and users since it is completely different from what is commonly done in the Julia package ecosystem. Whereas, e.g., there exist already tools for JuliaFormatter in VSCode, Atom, Emacs, and Vim. |
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
===========================================
- Coverage 93.13% 70.78% -22.35%
===========================================
Files 52 52
Lines 1252 1246 -6
===========================================
- Hits 1166 882 -284
- Misses 86 364 +278
Continue to review full report at Codecov.
|
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 can't see any harm in merging this as-is.
So far only contains initial notes on how to make a release (thanks @theogf for the explanation). Just to get the ball rolling. Please feel free to push to this branch and add to it!