Document our overall design principles, and guidelines for contribution

[sgrif]

There's a lot of implicit knowledge in much of our internals. While I try to give a lot of context to decision making in my commit messages, and you can probably figure out why a line is a certain way by git blaming it (and maybe looking at the parents of that line), but we should probably write down the most important bits. I will likely comment on this periodically with bits I think are important as I think of them, but I really don't know where we should document them

[sgrif]

Type Constraints

Due to the nature of what we're doing, we often have data structures that need to constrain their types in several different ways. Let's take Eq<T, U> as an example.

Eq<T, U> as a data structure can reasonably exist for any values of T and U. Therefore, the actual struct definition of T and U should not have any constraints on the struct definition itself, or its inherent impl.

Eq<T, U> is only a valid expression if T and U are the same SQL type. Therefore, in the impl Expression, we should constrain that T: Expression, U: Expression<SqlType=T::SqlType>. This should be loosened as needed based on the various usages of the expression. Traits like AsExpression should never appear on the struct itself. If we want to have a DSL which takes U: AsExpression<T::SqlType>, those sorts of constraints should appear on the DSL itself (the GlobalExpressionMethods#eq method in this case)

Constraints required for conversion to SQL should appear on the impl for QueryFragment. The impl for QueryFragment should not care about type safety, as that has been handled at higher levels. Eq<T, U> would likely only have the constraints that T: QueryFragment and U: QueryFragment.

Any type that implements Expression should likely implement NonAggregate and SelectableExpression, based on the implementations of its members. See the body of the infix_predicate! macro for common examples.

[sgrif]

Some Style Guidelines

One of the biggest lessons I've learned from maintaining Rails is that git history is really important, particularly the ability to get context on a line of code quickly via git blame. As such, much of the code is styled in such a way that lines are unlikely to change in order to support unrelated changes. Here's a few points that have come out of it. Note that these are guidelines, not hard rules. Use your best judgement.

If a function, struct, trait, or any other definition spans more than one line, the { goes on its own line.

Example:

// GOOD
fn foo(some_args: OmgVerboseTypeName, more_args: OmgVerboseTypeTwo)
    -> OurReturnType
{
    // ...
}

// BAD
fn foo(some_args: OmgVerboseTypeName, more_args: OmgVerboseTypeTwo)
    -> OurReturnType {
    // ...
}

// ALSO BAD
fn foo(some_args: OmgVerboseTypeName, more_args: OmgVerboseTypeTwo)
    -> OurReturnType {
        // ...
    }

The reasoning for this is simple: While I hate the curly brace being on its own line, I hate having part of the function signature being at the same indentation level as the function body (I need an easily identifiable visual separator), and we need the indentation level of the function body to be independent of the struct definition.

Do not token align. Just don't.

It rarely actually improves readability. The next line should start 4 spaces deeper than the previous, at the same indentation level, or 4 spaces shallower. Not 7 because it lets a : line up, as we'll have to re-align everything as soon as we add a longer key.

Always use trailing commas for multiline groupings

Example:

// GOOD
let ints = [
    1,
    2,
    3,
]

// BAD
let ints = [
    1,
    2,
    3
]

Keep lines under ~80 characters

You don't need to break it up when it's 81. My rough guideline is "does this fit in my terminal pane", which for me ends up being 1 vertical vim split out of 2 on a 15-inch macbook, and 1 vertical vim split out of 3 on a cinema display. This comes out to roughly 80ish characters, but there's no hard character limit. Documentation should have a hard wrap at 80 characters.

Prefer where clauses to the compact form basically always

Type constraints have a tendency to change more frequently than anything else. This will probably settle down a bit as we approach 1.0, but when a single constraint changes, I don't want to cause churn on the entire signature every time. My general rule of thumb is that you should use a where clause if there is more than one type parameter (even if all but one are unconstrained), or if the type parameter has more than one bound.

Break things into multi-line when it gets close to being required, not after

If a function signature is pushing it, it's reasonably likely something will change that pushes it over the
edge. Move to a where clause or multiline signature sooner rather than later.

State what you mean in where clauses

For example, if we still had Expression: QueryFragment, and we had

impl<T, U> QueryFragment for Bound<T, U> where
    T: NativeSqlType,
    U: ToSql<T>,
{
    // ...
}

then your constraint for Expression should show that you're trying to satisfy the constraints for QueryFragment, not just repeating them outright.

// GOOD
impl<T, U> Expression for Bound<T, U> where
    T: NativeSqlType,
    Bound<T, U>: QueryFragment,
{
    type SqlType = T;
}

// BAD
impl<T, U> Expression for Bound<T, U> where
    T: NativeSqlType,
    U: ToSql<T>
{
    type SqlType = T;
}

Here's what a long function signature should be structured like

fn really_long_function_name_omg_we_used_half_our_char_limit<T, U, V>(
    arg1: Type1,
    arg2: Type2,
) -> ReturnType where
    T: Something,
    U: Something,
{
    // body
}

If you find existing code that doesn't already adhere to these guidelines, don't change it simply to adhere to them

If we start changing code for style reasons only, it completely defeats the purpose of trying to avoid git churn in the first place. Do change code that doesn't adhere to these guidelines if you're already changing that line for other reasons.

[sgrif]

Common Abbreviations

ST: Sql Type. Basically always has the NativeSqlType constraint
DB: Database. Basically always has the Backend constraint.
QS: Query Source. Usually doesn't have a constraint, but sometimes will have QuerySource attached
PK: Primary Key
Lhs: Left Hand Side
Rhs: Right Hand Side
Conn: Connection

Generally, we prefer to give our types meaningful names. Lhs and Rhs vs T and U for a binary expression, for example.

[sgrif]

Our query builder (high level sense, not constructing sql sense) is Expression and AsExpression. QueryFragment is our AST.

There's a lot of overlap between these types. In a traditional OO language I would separate them more. Rust's type system means we don't have to, but I want to draw that distinction.

[YetAnotherMinion]

Would copy pasting this information to the GitHub wiki make sense? I am completely unfamiliar with the GitHub API, so if putting information in issues makes it easier to programmatically export it from GitHub then I retract. I saw that the wiki has zero pages and that made me pause and come back to ask for permission.

edit: Or as it occurred to me, just put the markdown files into the repository itself, perhaps in a brand new contributing or meta-doc directory.

[killercup]

@YetAnotherMinion I'd rather put it in markdown file in the repo, like contributing.md.