Update Rust style guide #3172

wdanilo · 2021-11-30T17:35:56Z

I've created a bunch of small updates to the style guide which describe things that we already use in the codebase. Is there anything missing, or is something not clear enough? If so, please write comments about that and I'll fix it.

vitvakatu · 2021-11-30T19:51:40Z

Thank you, I love this changes!

I would like to have answers to the following questions:

What can be placed inside a single section/subsection and what cannot? Can I place structure definition and its impl in separate subsections? Can I place a helper structure definition and a set of constants in a single section without subsections (see this)?
What items should be documented with /// docstrings and what shouldn't? Should I document obvious-looking methods? (we have exactly 400 /// Constructor. in our codebase) Should I document structure fields or enum variants? Should I document module-private items? Should I document every public one?
Is there some kind of policy for TODO and FIXME comments? What if I really-really want to add such comment with issue link?

mwu-tow · 2021-12-01T00:57:52Z

app/gui/docs/contributing/style-guide.md

+- **Use one import per line.**  
+  Do not use the `use crate::mod_name::{name1, name2, name3}` syntax. For each entity use a separate `use` statement.


This is enforced by the rustfmt, likely no need to include this here,

mwu-tow · 2021-12-01T02:54:04Z

app/gui/docs/contributing/style-guide.md

+- **Group related submodule and import statements**  
+  Submodule and import statements should be divided into several groups separated by blank lines. Items in the groups should be sorted alphabetically (do not use group comments in your code):


This is not what I've been already doing nor what I've been requesting in my reviews, so I think it actually warrants some discussion.
Also, this should be unified with https://github.com/enso-org/ide/blob/develop/docs/contributing/style-guide.md
Ideally, both style guides should be unified, as we have a single repository now.
Merging as-is would create a conflict between these documents.

Also, this should be unified with https://github.com/enso-org/ide/blob/develop/docs/contributing/style-guide.md
Ideally, both style guides should be unified, as we have a single repository now.
Merging as-is would create a conflict between these documents.

But the enso-org/ide is archived, right? That strongly suggests, that the guidelines there should not be used anymore.

Ouch, wrong link.
The one I've meant is https://github.com/enso-org/enso/blob/develop/docs/style-guide/rust.md

Issue is that we have two Rust style guide documents.

The idea is to remove the second style guide ASAP and make this the only one.

farmaazon · 2021-12-01T08:24:11Z

app/gui/docs/contributing/style-guide.md

+- **Don't use relative imports.**  
+  Do not use `super::` nor `self::` imports. Use absolute imports or imports from local submodules only. 


I'd add an exception for "inlined" modules, or at least for the "test" module.

farmaazon · 2021-12-01T08:30:11Z

app/gui/docs/contributing/style-guide.md

+  // Reexports.
+  pub use geometry::*;


I'd put reexports as the last group: sometimes you reexport an imported thing (not necessarily from a submodule). Additionally, we have then a clear place where imports ends, and visible names starts (reexports, and then definitions).

Publicly available things should be next to each other, so, re-exports should ld be next to sub-modules. Otherwise, you need to brwose different parts of the files in order to understand what are the publicly exposed entities in the file.

But still, the definitions are also publicly available. So in the current state you have:

public submodules

public reexports

non-public imports

public definitions

That's why I opted for moving 2 after 3.

On the other hand... imports might be treated as part of 4 actually, so let's stay with your ordering!

farmaazon · 2021-12-01T08:34:20Z

app/gui/docs/contributing/style-guide.md

+  // Prelude-like imports.
+  use crate::prelude::*;
+  use sprite_system::traits::*;
+
+  // Local-crate imports.
+  use crate::closure;
+  use crate::data::opt_vec::OptVec;
+  use crate::data::dirty;
+  use crate::system::web::group;
+
+  // External imports.
+  use nalgebra::Matrix4;
+  use nalgebra::Vector3;


What prelude-like means? Everything with star at the end?

By the way, once we have rust formatter, I wonder if we could keep imports in a single block, sorted alphabetically. Personally, I've never taken an advantage of having imports separated in blocks. But having a single block allows me to make use of Intellij auto-import feature + that makes mass-refactoring easier as well.

Let's chat about it. I think this needs a more interactive discussion.

Did you end up talking about this? If so, what was the result of the discussion?

farmaazon · 2021-12-01T08:36:18Z

app/gui/docs/contributing/style-guide.md

+- **Group related submodule and import statements**  
+  Submodule and import statements should be divided into several groups separated by blank lines. Items in the groups should be sorted alphabetically (do not use group comments in your code):


Also, this should be unified with https://github.com/enso-org/ide/blob/develop/docs/contributing/style-guide.md
Ideally, both style guides should be unified, as we have a single repository now.
Merging as-is would create a conflict between these documents.

But the enso-org/ide is archived, right? That strongly suggests, that the guidelines there should not be used anymore.

wdanilo · 2021-12-02T07:18:17Z

@vitvakatu, good points! I have a few more, based on yesterdays PR review, I plan to add these + answers to your questions somewhat today / tomorrow evening!

MichaelMauderer · 2021-12-20T10:19:43Z

@wdanilo This has been open for a while without change. What is the state of the updates?

vitvakatu · 2022-02-15T09:41:53Z

Another thing that I faced - we do not have any explicit requirements for code formatting inside macros (like FRP macros or f! macro) - although our codebase definitely preserves some "historical" code formatting there.

wdanilo · 2022-04-13T04:43:15Z

Everyone, I apologise for making this waiting so long. I addressed the comments.

@vitvakatu, regarding your points 1-3 (the first comment), let's create separate PRs with the changes there and let's discuss these things on Discord first.

MichaelMauderer · 2022-04-13T08:29:15Z

app/gui/docs/contributing/style-guide.md

+  Do not use `super::` nor `self::` imports in files (you can use them in localy defined modules). Use absolute imports or imports from local submodules only.
+
+- **Use Enso Formatter to format your imports**  
+  Run the `build/enso-formatter` script to format imports in all files before contributing your PR.


Can we add a copy-pastable command for running this from the project root?

Yeah, that would be nice. I use cargo run -p enso-formatter for that. Even nicer would be to integrate it with our ./run lint tool, but I remember there were some problems with that.

vitvakatu

I think the changes in this PR are rather subtle and do not answer a lot of practical questions around our code style, but let's discuss them separately as this PR is here for a few months now.

vitvakatu · 2022-04-13T09:56:32Z

app/gui/docs/contributing/style-guide.md


-## Styling rules
+## Code formatting in macros.
+Unfortunately, `rustfmt` is not working inside of macros. Thus, this code should be manually formated in the same was as `rustfmt` would do it.


Suggested change

Unfortunately, `rustfmt` is not working inside of macros. Thus, this code should be manually formated in the same was as `rustfmt` would do it.

Unfortunately, `rustfmt` is not working inside of macros. Thus, this code should be manually formated in the same way as `rustfmt` would do it.

However, it's not clear how "rustfmt would do it" for non-Rust syntax, like our FRP definitions.

vitvakatu · 2022-04-13T09:57:45Z

app/gui/docs/contributing/style-guide.md

+  Do not use `super::` nor `self::` imports in files (you can use them in localy defined modules). Use absolute imports or imports from local submodules only.
+
+- **Use Enso Formatter to format your imports**  
+  Run the `build/enso-formatter` script to format imports in all files before contributing your PR.


Yeah, that would be nice. I use cargo run -p enso-formatter for that. Even nicer would be to integrate it with our ./run lint tool, but I remember there were some problems with that.

Update Rust style guide

c3417e9

wdanilo requested review from farmaazon, MichaelMauderer and mwu-tow as code owners November 30, 2021 17:35

wdanilo requested a review from vitvakatu November 30, 2021 17:36

mwu-tow suggested changes Dec 1, 2021

View reviewed changes

farmaazon reviewed Dec 1, 2021

View reviewed changes

vitvakatu mentioned this pull request Feb 15, 2022

Restrict maximum zoom in Graph Editor #3273

Merged

4 tasks

wdanilo added 2 commits April 13, 2022 06:36

Merge branch 'develop' into wip/wdanilo/doc-fixes

64a40a1

Fixes

08a67a9

MichaelMauderer reviewed Apr 13, 2022

View reviewed changes

vitvakatu approved these changes Apr 13, 2022

View reviewed changes

wdanilo added 2 commits April 15, 2022 16:10

Update style-guide.md

c9d4d7c

Merge branch 'develop' into wip/wdanilo/doc-fixes

3fb889e

wdanilo merged commit e8dfc6d into develop Apr 15, 2022

wdanilo deleted the wip/wdanilo/doc-fixes branch April 15, 2022 14:10

hubertp pushed a commit that referenced this pull request Apr 19, 2022

Update Rust style guide (#3172)

de79819

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Rust style guide #3172

Update Rust style guide #3172

wdanilo commented Nov 30, 2021

vitvakatu commented Nov 30, 2021

mwu-tow Dec 1, 2021

mwu-tow Dec 1, 2021

farmaazon Dec 1, 2021

mwu-tow Dec 1, 2021

wdanilo Dec 2, 2021

farmaazon Dec 1, 2021

farmaazon Dec 1, 2021

wdanilo Dec 2, 2021

farmaazon Dec 2, 2021

farmaazon Dec 1, 2021

wdanilo Dec 2, 2021

MichaelMauderer Dec 13, 2021

farmaazon Dec 1, 2021

wdanilo commented Dec 2, 2021

MichaelMauderer commented Dec 20, 2021

vitvakatu commented Feb 15, 2022

wdanilo commented Apr 13, 2022

MichaelMauderer Apr 13, 2022

vitvakatu Apr 13, 2022

vitvakatu left a comment

vitvakatu Apr 13, 2022

vitvakatu Apr 13, 2022

		- Use one import per line.
		Do not use the `use crate::mod_name::{name1, name2, name3}` syntax. For each entity use a separate `use` statement.

		- Group related submodule and import statements
		Submodule and import statements should be divided into several groups separated by blank lines. Items in the groups should be sorted alphabetically (do not use group comments in your code):

		- Don't use relative imports.
		Do not use `super::` nor `self::` imports. Use absolute imports or imports from local submodules only.

	Unfortunately, `rustfmt` is not working inside of macros. Thus, this code should be manually formated in the same was as `rustfmt` would do it.
	Unfortunately, `rustfmt` is not working inside of macros. Thus, this code should be manually formated in the same way as `rustfmt` would do it.

Update Rust style guide #3172

Update Rust style guide #3172

Conversation

wdanilo commented Nov 30, 2021

vitvakatu commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdanilo commented Dec 2, 2021

MichaelMauderer commented Dec 20, 2021

vitvakatu commented Feb 15, 2022

wdanilo commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment