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

RFC: Resolve numerous naming conventions #344

Merged
merged 6 commits into from
Oct 15, 2014

Conversation

aturon
Copy link
Member

@aturon aturon commented Oct 1, 2014

This is a conventions RFC for settling a number of remaining naming conventions:

  • Referring to types in method names
  • Iterator type names
  • Additional iterator method names
  • Getter/setter APIs
  • Associated types
  • Trait naming
  • Lint naming
  • Suffix ordering
  • Prelude traits

It also proposes to standardize on lower case error messages within the compiler
and standard library.

Rendered

This RFC proposes to amend the convention to further say: if there is a single
method that is the dominant functionality of the trait, consider using the same
name for the trait itself. This is already the case for `Clone` and `Show`, for
example.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show doesn't quite fit into this category because its method is called fmt due to sticking with the other formatting traits. (not sure if that affects this convention)

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@alexcrichton nits addressed.

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

I am especially interested in feedback on the trait naming convention, which I think really affects the terse feel of Rust but which is still pretty vague.

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

Update: I've added two new sections after another round of API stabilization: additional iterator method names, and prelude traits.

cc @brson @huonw @alexcrichton @nikomatsakis

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@arcto

Traits such as Copy Clone Show ToCStr each describes a specific capability. With this in mind Reader would be confusing if shortened to Read.

Yes, the convention would be maximally clear if it only applied to single-method/capability traits. However, I'd argue that for Reader the read method is the "primary" one in that everything else has a default implementation. (Though, by the same argument, Iterator should be Next, which is... very bad.)

I think you're probably right about Reader. So a general question would be: when is the -er suffix appropriate?

@arcto
Copy link

arcto commented Oct 1, 2014

Would ReadStream be better than just Read? (just thinking aloud)

@arcto
Copy link

arcto commented Oct 1, 2014

Iterator is the established name. The capability, in my mind, is GetNext. However, in getters the get_ part is omitted, so what's left is just next(). Not arguing for a specific trait name, though.

@Gankra
Copy link
Contributor

Gankra commented Oct 1, 2014

This naming scheme can result in clashes if multiple containers are defined in the same module. Note that this is already the case with today's conventions. In most cases, this situation should be taken as an indication that a more refined module hierarchy is called for.

We're going to have to rework how we do re-exports in libcollections then. See e.g. btree

@alexcrichton
Copy link
Member

I'm a little wary about the *-Prelude convention as we have so little experience with it so far, but I suspect that it will turn out alright.

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@gankro

We're going to have to rework how we do re-exports in libcollections then. See e.g. btree

This is primarily due to the map and set being flattened into a single module, right? I think separating these out was a change we wanted to make anyway.

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@alexcrichton

I'm a little wary about the *-Prelude convention as we have so little experience with it so far, but I suspect that it will turn out alright.

It should be the case that these name choices are totally irrelevant to the vast majority of Rust code. But maybe it's worth prototyping these changes to check that's actually the case.

@Gankra
Copy link
Contributor

Gankra commented Oct 1, 2014

@aturon btree's map/set are separated out into separate modules, but the parent btree module re-exports things for convenience (same story for the hashmap/set modules).

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@gankro Right. I think maybe we weren't on the same page about what the final state of these would look like. Personally, in the end, I'd prefer for e.g. btree's map and set to show up at the root level as completely independent modules.

@Gankra
Copy link
Contributor

Gankra commented Oct 1, 2014

@aturon Would there still be a parent btree module in reality, with the modules just re-exported to the root of libcollections? Or would they both be root-level modules in the actual source tree?

@carllerche
Copy link
Member

I don't see anything regarding builder style, which I enjoy:

For example, in my curl bindings:

http::handle()
  .get("http://www.example.com")
  .header("Authorization", "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==")
  .exec();

The thing about it is that the builder style usually just sets values on a builder struct, but doesn't follow the set_foo convention.

@reem
Copy link

reem commented Oct 1, 2014

Agree with @carllerche. The builder pattern is extremely useful and very flexible.

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@carllerche @reem Oh, definitely! (There's a section on builders in the guidelines already).

I'll add a clarification that this new convention does not apply to builders.

It's worth thinking a bit about whether builders should expose getters as well (opinions vary) and if so, whether they just follow the reverse convention: foo to set, get_foo to get?

@aturon
Copy link
Member Author

aturon commented Oct 1, 2014

@carllerche @reem Pushed an update to clarify.

I think I'd like to handle builder conventions via a separate RFC -- in particular, I'd like to put the guidelines I wrote earlier through the RFC process. Right now it's mainly a description of how this played out for Command and TaskBuilder.

@Ericson2314
Copy link
Contributor

Read / Write sound good to me. Maybe Iterator could become Iterate, though not sure where that leaves Iterable.

This is primarily due to the map and set being flattened into a single module, right? I think separating these out was a change we wanted to make anyway.

Quiet unrelated to the RFC, but it would be nice if the backing data structure and it's core functions were in a separate file from the, e.g., *Map and *Set public data structures and their many bland impls. I've multiple times gone to look at the collections implementations and found it annoying to wade through 1000+ lines trying to find the interesting bits.

@reem
Copy link

reem commented Oct 2, 2014

Read and Write makes me think that the type itself can be read or written, as in the case of something parseable and showable. Reader, Writer and Iterator are much clearer in this case.

@arcto
Copy link

arcto commented Oct 2, 2014

Here's a nice naming convention for getters and setters, common in C++. But it requires function overloading:

fn name() -> &str;
fn name(s: &str);

This one is also compatible with builders.

@petrochenkov
Copy link
Contributor

@arcto These "nice" getters and setters can't be distinguished with simple text search and aren't encouraged by popular style guides.

@aturon
Copy link
Member Author

aturon commented Oct 14, 2014

Note: I've pushed a minor update to the lint naming rules, reflecting my experience actually rolling out the conventions. Other than clarifying the existing rules, the main change is standardizing on "unused" (where we previously had "unnecessary", "unused" and "useless"). This matches the unused umbrella lint, as well.

`&T` | `ref`
`&mut T` | `mut`
`*const T`| `ptr`
`*mut T` | `mut_ptr`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about [T, ..N]? array? fixed? In cgmath we use fixed.

Should tuples be considered as well? Or should we wait for those conventions to emerge naturally for now? (I haven't seen many tuple conversions yet).

@nikomatsakis
Copy link
Contributor

I think the reason that Reader being called Read seems odd to me is that read() doesn't feel like the primary method of Reader to me -- there doesn't seem to be a "primary" method.

bors added a commit to rust-lang/rust that referenced this pull request Oct 15, 2014
[RFC 344](rust-lang/rfcs#344) proposes a set of naming conventions for lints. This PR
renames existing lints to follow the conventions.

Use the following sed script to bring your code up to date:

```
s/unnecessary_typecast/unused_typecasts/g
s/unsigned_negate/unsigned_negation/g
s/type_limits/unused_comparisons/g
s/type_overflow/overflowing_literals/g
s/ctypes/improper_ctypes/g
s/owned_heap_memory/box_pointers/g
s/unused_attribute/unused_attributes/g
s/path_statement/path_statements/g
s/unused_must_use/unused_must_use/g
s/unused_result/unused_results/g
s/non_uppercase_statics/non_upper_case_globals/g
s/unnecessary_parens/unused_parens/g
s/unnecessary_import_braces/unused_import_braces/g
s/unused_unsafe/unused_unsafe/g
s/unsafe_block/unsafe_blocks/g
s/unused_mut/unused_mut/g
s/unnecessary_allocation/unused_allocation/g
s/missing_doc/missing_docs/g
s/unused_imports/unused_imports/g
s/unused_extern_crate/unused_extern_crates/g
s/unnecessary_qualification/unused_qualifications/g
s/unrecognized_lint/unknown_lints/g
s/unused_variable/unused_variables/g
s/dead_assignment/unused_assignments/g
s/unknown_crate_type/unknown_crate_types/g
s/variant_size_difference/variant_size_differences/g
s/transmute_fat_ptr/fat_ptr_transmutes/g
```

Since a large number of lints are being renamed for RFC 344, this PR
adds some basic deprecation/renaming functionality to the pluggable lint
system. It allows a simple mapping of old to new names, and can warn
when old names are being used.

This change needs to be rolled out in stages. In this PR, the
deprecation warning is commented out, but the old name is forwarded to
the new one.

Once the PR lands and we have generated a new snapshot of the
compiler, we can add the deprecation warning and rename all uses of the
lints in the rust codebase. I will file a PR to do so.

Closes #16545
Closes #17932

r? @pcwalton
@alexcrichton alexcrichton merged commit eb5e363 into rust-lang:master Oct 15, 2014
aturon added a commit to aturon/rust that referenced this pull request Nov 6, 2014
This commit renames a number of extension traits for slices and string
slices, now that they have been refactored for DST. In many cases,
multiple extension traits could now be consolidated. Further
consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude`
suffix](rust-lang/rfcs#344). There are probably
a few more candidates for being renamed this way, but that is left for
API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard
library to actually break.

Closes rust-lang#17917
bors added a commit to rust-lang/rust that referenced this pull request Nov 6, 2014
This commit renames a number of extension traits for slices and string slices, now that they have been refactored for DST. In many cases, multiple extension traits could now be consolidated. Further consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude` suffix](rust-lang/rfcs#344). There are probably a few more candidates for being renamed this way, but that is left for API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard library to actually break.

Closes #17917
SimonSapin pushed a commit to tomprogrammer/rust-ascii that referenced this pull request Nov 27, 2014
This commit renames a number of extension traits for slices and string
slices, now that they have been refactored for DST. In many cases,
multiple extension traits could now be consolidated. Further
consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude`
suffix](rust-lang/rfcs#344). There are probably
a few more candidates for being renamed this way, but that is left for
API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard
library to actually break.

Closes #17917
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 30, 2014
Rename struct `Entries` to `Iter` in hash/table.rs and hash/map.rs, to match the naming convention of rust-lang/rfcs#344.

This is a [breaking-change].
cuviper pushed a commit to cuviper/rayon that referenced this pull request Mar 28, 2017
This commit renames a number of extension traits for slices and string
slices, now that they have been refactored for DST. In many cases,
multiple extension traits could now be consolidated. Further
consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude`
suffix](rust-lang/rfcs#344). There are probably
a few more candidates for being renamed this way, but that is left for
API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard
library to actually break.

Closes #17917
cuviper pushed a commit to cuviper/rayon that referenced this pull request Mar 28, 2017
Rename struct `Entries` to `Iter` in hash/table.rs and hash/map.rs, to match the naming convention of rust-lang/rfcs#344.

This is a [breaking-change].
wycats pushed a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019
Twey added a commit to Twey/linera-protocol that referenced this pull request Jul 2, 2024
This is more in line both with Rust standard library traits, which
tend to be transitive verbs, and also with the Rust trait naming
conventions RFC rust-lang/rfcs#344, which states:

> Prefer (transitive) verbs, nouns, and then adjectives; avoid
> grammatical suffixes (like able).
Twey added a commit to linera-io/linera-protocol that referenced this pull request Jul 3, 2024
* `linera-core`: add `Persistent` trait

* `linera-service`: use new `Persistent` trait

* `linera-service`: don't forget to create the file for `persistent::File::new`

* `linera-service`: refresh PRNG seed more to align with tests

* `linera-service`: rename `Persistent` to `Persist`

This is more in line both with Rust standard library traits, which
tend to be transitive verbs, and also with the Rust trait naming
conventions RFC rust-lang/rfcs#344, which states:

> Prefer (transitive) verbs, nouns, and then adjectives; avoid
> grammatical suffixes (like able).

* `linera-service`: document `Persist` trait

* `linera-service::persistent::File`: when an error occurs during error handling, do not hide the outer error

* `linera_service::persistent`: single-space documentation

Co-authored-by: Andreas Fackler <[email protected]>
Signed-off-by: James Kay <[email protected]>

* `linera-service`: mention bug #2053

* `linera_service::proxy`: replace `expect` with `?`

* `linera_service::persistent`: use the indicative mood for function documentation

---------

Signed-off-by: James Kay <[email protected]>
Co-authored-by: Andreas Fackler <[email protected]>
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
This commit renames a number of extension traits for slices and string
slices, now that they have been refactored for DST. In many cases,
multiple extension traits could now be consolidated. Further
consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude`
suffix](rust-lang/rfcs#344). There are probably
a few more candidates for being renamed this way, but that is left for
API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard
library to actually break.

Closes #17917
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
This commit renames a number of extension traits for slices and string slices, now that they have been refactored for DST. In many cases, multiple extension traits could now be consolidated. Further consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude` suffix](rust-lang/rfcs#344). There are probably a few more candidates for being renamed this way, but that is left for API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard library to actually break.

Closes #17917
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.