Conversation
I figure it's best to have this inside contributing guidelines than on our wiki because it'll be closer to the source - when someone wants to contribute, they'll see the guidelines right here and then without having to go on the wiki. Plus, this'll also enforce team agreement for new guidelines
ahmedcharles
left a comment
There was a problem hiding this comment.
My only other general comment is that this seems like QT_CODING_GUIDELINES.md, until we have more general info. :)
| @@ -0,0 +1,41 @@ | |||
| # Coding guidelines | |||
There was a problem hiding this comment.
The file should probably be named CODING_GUIDELINES.md rather than CONTRIBUTING.md. This file should probably point to that way and say that we expect you to follow it.
There was a problem hiding this comment.
I'd rather avoid bouncing people around all sorts of links - how about until CONTRIBUTING.md gets too big, we keep it in one file?
There was a problem hiding this comment.
Sure, but then I'd expect this file to explain some high level things about how to contribute. Right now, the content matches a file with a different name. If we're just going to have guidelines then just call it that.
There was a problem hiding this comment.
I don't have any ideas for general guidelines yet, but I did come up with some coding guidelines and dumped them in for a start. Happy to take general guideline suggestions into the file as well - while keeping the obvious stuff out to avoid losing our reader.
There was a problem hiding this comment.
The primary reason for the filename is that Github will show a little green bar when making a PR - so the person can just click on on it and tada, see the coding guidelines here and then. Developer experience is very important to me because they're humans too.
There was a problem hiding this comment.
I'm aware of that.
But my bar for a file that gets that link is higher than this file currently meets. For example: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md
That file has the following sub-sections:
- Feature Requests
- Bug Reports
- The Build System
- Pull Requests
- Writing Documentation
- Issue Triage
- Out-of-tree Contributions
- Helpful Links and Information
That's sort of what I expect when I click the bar that GitHub helpfully shows. I don't expect "here's three gotchas about QStringLiteral".
There was a problem hiding this comment.
I've taken your feedback on board and added the Bug Reports category. I couldn't find a need for the rest given the context of someone reading that file when they're filing an issue or submitting a PR - let me know if you'd like to see any specific categories.
In general, I find that file to be overwhelming for a first-time contributor who just wants to submit a 1 line fix and you have to remember that the scale of Rust and Mudlet are totally different, so will be the people who contribute to it (hardcore C++ experts for Rust vs people who learnt coding copy/pasting code in Mudlet for us).
There was a problem hiding this comment.
I think we could use a Feature Request section as well, if it's any different than the Bug Reports section. Having a Pull Request section also seems useful (we could explain the labelling and the code review groups). There's also documentation for mudlet though I don't even know how to edit it, so that section seems really relevant. Build and Test sections should hopefully become relevant once we have more testing. :)
I'm not going to suggest that we have all of these right now, nor would I suggest just copying the Rust one. For example, we could have sections for Lua vs C++ contributions.
As for the more general comment about the scale of Rust vs Mudlet, I agree that the word scale (meaning number of contributors, etc) would have them be different. But the implication of the rest of your comment is that Rust targets a different set of developers when getting contributions and that simply isn't true. Their goal is to get contributions from a wide variety of people with all sorts of backgrounds. Their primary documentation writer is a former ruby on rails programmer, for example, not a C++ expert. Just because I'm a C++ expert doesn't mean that everything I advocate for is to benefit only C++ experts. I'd prefer if my suggestions not meet that particular objection over and over.
There was a problem hiding this comment.
I'm not objecting to your suggestions based on your background don't take it personally. I'm just saying people who contribute to a programming language (very, very, very intimidating) are gonna be different who contribute to a MUD client (very intimidating). It's certainly true that Rust would like contributors of all sorts but I think in practice what they attract will be higher-grade caliber programmers compared to what we'll attract. Your background in fact is very much appreciated here!
I've added Feature Requests - let me know what you think of it now.
There was a problem hiding this comment.
I wasn't taking it personally (I think). I was just trying to point out that while I contribute to Rust and I am a C++ expert, that I've seen many people who contribute to Rust who are much closer to the "copy paste code into Mudlet" side of things. In fact, that's the appeal of Rust, the fact that the compiler stops you from getting lots of things wrong. I'd recommend not assuming things about the Rust community when compared to the Mudlet community just because it's a programming language.
Anyways, I'm mostly fine with this, as a start. Hopefully we can expand on it over time.
ahmedcharles
left a comment
There was a problem hiding this comment.
Rebase/squash/etc. please.
|
This review ui is weird, I'll have to get used to it some more. |
|
|
||
| If you're a first-timer, you're excluded, we'll go easy on you :wink: | ||
|
|
||
| ## Use QLatin1String over QStringLiteral if possible |
There was a problem hiding this comment.
Oh bottom I wasn't aware of that - guess it must be the thing I count as "something I learnt today".
There was a problem hiding this comment.
Oh, QLatin1String does not support replaceable inserts like QStringLiteral does, (no QLatin1String::arg(...) methods) so you will just have to build up a (non-UI) string from its component parts - but have an #include <QStringBuilder> in the compilation unit and you can use the % (compile time?) string concatenation operator which is more efficient than the + as a whole load of them in one code block only causes one memory allocation/copy operation rather than one per + - if I understand it correctly...
| # Feature Requests | ||
| Want a new feature in Mudlet? Let us know about it! We also have a long-running feature requests thread on [our forums](http://forums.mudlet.org/viewtopic.php?f=5&t=92). | ||
|
|
||
| # Bug Reports |
There was a problem hiding this comment.
Do we actually need to say something about how to raise a bug report - i.e. raise an issue here about it?
There was a problem hiding this comment.
Nah, because people will see this file after they've clicked on "New Issue" already.
|
I think we also need to include pointers to the Development area of the Forums, and/or include some salient posts/topics or extracts thereof: e.g.: http://forums.mudlet.org/viewtopic.php?f=7&t=4619 and http://forums.mudlet.org/viewtopic.php?f=7&t=9043...? |
|
I don't think that's necessary because they see this by the time they've already setup and are on their way to submit the PR. |
|
Strange - I added a comment on github yet it didn't appear. @SlySven anything else you'd like to see? |
|
Well, to avoid the problems AC ran into a while back 😉 we might have to remind contributors that we are doing stuff under GPL (or MIT for Lua stuff) conditions - that sort of automatically applies for stuff on public GitHub repositories anyhow, but contributors have to be aware that they have to be able to give what code they want to give to the project without compromising themselves or it in the process... |
keneanung
left a comment
There was a problem hiding this comment.
Looks good! Do we want to add something about how we want git commits or git timeline to look (if/when we get a common denominator)?
|
@SlySven It's an issue we've had once in our 8 years of running the project - don't think it deserves a mention yet. If it becomes a persistent issue, sure, will add it - but it hasn't exactly been a pain point for us that deserves special attention. |
|
At this point, I just want to get this in so there are less open PR's and we can move on to discussing other ones. 🚀 |
I figure it's best to have this inside contributing guidelines than on our wiki because it'll be closer to the source - when someone wants to contribute, they'll see the guidelines right here and then without having to go on the wiki.
Plus, this'll also enforce team agreement for new guidelines.
Tagging @Mudlet/core-cpp for review.