Improving Code Idiomacy #268

koopa1338 · 2023-11-19T22:00:46Z

This pull request focuses on enhancing the idiomatic expression within the Rust codebase. By adhering more closely to Rust's best practices and conventions, this update aims to improve readability, maintainability, and performance where applicable.

Changes Made

Iterator Usage: Utilized iterators and functional programming paradigms in certain loops, enhancing code clarity.
Options: Utilize methods of options to avoid unnecessary if-else
Constructors: Removed empty constructors and replaced them with their Default implementation
Traits: Where possible, used impl Traits in function arguments and replaced methods with their semantic trait implementation
Derives: Sorted derives and add Default where possible
Modules: Replaced named module files that contained only module declaration with corresponding mod.rs files

Sharktheone · 2023-11-19T22:35:52Z

For point 6: We have moved from the mod.rs syntax in #107. It can get quite messy and chaotic when having 100 files called mod.rs, especially when you have multiple of them open in your editor.

Kiyoshika · 2023-11-20T04:03:38Z

src/testing/tree_construction/fixture.rs

-pub fn read_fixture_from_path(path: &PathBuf) -> Result<FixtureFile, Error> {
-    let input = fs::read_to_string(path)?;
-    let path = path.to_string_lossy().into_owned();
+pub fn read_fixture_from_path(path: impl AsRef<Path>) -> Result<FixtureFile, Error> {


Not that I'm disagreeing, but for my own knowledge (still new to rust) - what's the benefit of using AsRef here versus a "regular" & reference?

Read through the rust docs, I guess it's for anything that can be converted to a PathBuf reference. I'm not familiar enough with this portion of the code to know if that's done, but I don't see the harm

Read through the rust docs, I guess it's for anything that can be converted to a PathBuf reference. I'm not familiar enough with this portion of the code to know if that's done, but I don't see the harm

Exactly, I guess it isn't necessary at this point, I'm also not familiar with the code, so I only "fixed" things in a way that is common in Rust and I would also do myself. In this case it makes the api more flexible for future usage.

Sharktheone · 2023-11-20T16:02:34Z

It breaks many of the tests tough, and you may consider running make format and fixing some cargo clippy warnings

koopa1338 · 2023-11-20T18:45:28Z

yeah it's still wip, I will make sure that all tests run again and the ci is happy 😉

jaytaph · 2023-11-22T19:29:59Z

@koopa1338 sorry for the delay. Didn't see it was still pending. If we have approval, we can merge the PR. If you like, you can add your name to the AUTHORS file as well.

koopa1338 · 2023-11-22T20:52:19Z

No worries, I added myself to the authors file 😉

Kiyoshika · 2023-11-22T21:30:06Z

I looked through a good chunk of this earlier, will finish reviewing hopefully soon, trying to wrap up some of the bindings real quick

Kiyoshika · 2023-11-22T21:31:30Z

@jaytaph do you want the commits squashed prior to merging? I know lately we usually stick to 1-2 commits per PR

Sharktheone

I like most of the changes, especially adding the "missing" semicolons. But i really don't like the things with default. I personally prefer new over default.

Sharktheone · 2023-11-22T21:38:34Z

src/config.rs

I think, this will interfere with #267, as well as the other files, edited in this pr

Sharktheone · 2023-11-22T21:52:07Z

src/html5/node.rs

+        let (id, parent, children, name, namespace, is_registered) = <_>::default();
        Node {
-            id: Default::default(),
-            parent: None,
-            children: vec![],
+            id,
+            parent,
+            children,
            data: NodeData::Document(DocumentData::new()),
-            name: "".to_string(),
-            namespace: None,
+            name,
+            namespace,
            document: Document::clone(document),
-            is_registered: false,
+            is_registered,


I guess it would make sense to do this like this

Suggested change

let (id, parent, children, name, namespace, is_registered) = <_>::default();

Node {

id: Default::default(),

parent: None,

children: vec![],

id,

parent,

children,

data: NodeData::Document(DocumentData::new()),

name: "".to_string(),

namespace: None,

name,

namespace,

document: Document::clone(document),

is_registered: false,

is_registered,

Node {

data: NodeData::Document(DocumentData::new()),

document: Document::clone(document),

..Default::default()

}

You would need to implement Default for Node then, but this should be possible

I looked into it, and there are some issue with the default implementation that I couldn't solve, for example I would have to use a default for the NodeData and neither of the variants looks like a good fit for a default value imo. Any suggestions?

I guess, you could use a comment as a "Default" NodeData. But Document could also make sense, because it's an empty struct. I guess it's up to you, if no one as a other opinion on this.

I would skip that for now, the PR is already big enough and I guess there might be more discussion about that, so I will open an Issue regarding this.

Sharktheone · 2023-11-22T22:02:10Z

src/html5/parser.rs

@@ -3667,7 +3667,7 @@ impl<'chars> Html5Parser<'chars> {
                    let new_name = SVG_ADJUSTMENTS_ATTRIBUTES
                        .get(name)
                        .expect("svg adjustments");
-                    new_attributes.insert(new_name.to_string(), value.clone());
+                    new_attributes.insert((*new_name).to_owned(), value.clone());


This seems a bit odd, i mean defer the element and then call to_owned if you could just call to_string

Yeah, I can see that this is kind of weird, It would be fine to revert this to to_string, however, we should use to_owned if we can, because to_string will use the generic ToString implementation and therefor use the fomratting functions, which end up doing multiple allocations. The to_owned function just allocates a buffer and copies the literal into that buffer.

The deref is necessary because we get a &&str out of the hashmap. Another option that might be better to read would be this:

let &new_name = SVG_ADJUSTMENTS_ATTRIBUTES .get(name) .expect("svg adjustments"); new_attributes.insert(new_name.to_owned(), value.clone());

What do you think?

Oh, i must have missed this... Good to know. Thanks for the explanation. I guess it makes sense to do it like in the code snippet you provided.

Sharktheone · 2023-11-22T22:21:04Z

And can you maybe look at #107/#102 where we removed all the mod.rs files.

emwalker

@koopa1338 I like about 80 percent of the changes in this PR, and I can live with all of them. My main reservation is the size of the PR; in the future, perhaps tackling one thing at a time will make it easier to review.

Some of the changes are a little arbitrary, e.g., to_string() -> to_owned(), but I don't mind them, even if that's the case. This kind of update is a good way of familiarizing oneself with the code base.

For now, my own thought is to merge what you've done here. But first:

There are merge conflicts that will need to be manually resolved
Once the conflicts are resolved, you'll want to squash all commits into a single commit and force push

Sharktheone · 2023-11-23T19:04:53Z

Holy, this screenshot is like a flashbang... I can also live with all of the changes, but as I said above, I don't really like the "renaming" all new functions to default.

jaytaph · 2023-11-23T19:43:38Z

As far as I can tell, one of the benefits of having "Default" instead of New (considering new() would not have any arguments), is that it's easier to use them in scenario's like "unwrap_or_default() and such.

Sharktheone · 2023-11-23T20:13:12Z

I mean you can have both new and default, you can just call new in default

emwalker · 2023-11-23T23:33:10Z

I personally don't mind a new(), and I think new() can even delegate to default():

impl Something {
  fn new() -> Self {
    Self::default()
  }
}

My own thought on the changes in this PR is that they're a vague improvement, because we're getting Default defined, and we're using it in a bunch of places where it makes sense. And the PR allows another developer to learn the code. We can always backfill new() back selectively as we go when it makes sense. But I'm just one developer here, so I'm happy to go with whatever other people think.

koopa1338 · 2023-11-27T16:36:02Z

For now, my own thought is to merge what you've done here. But first:

There are merge conflicts that will need to be manually resolved

Once the conflicts are resolved, you'll want to squash all commits into a single commit and force push

I would do another rebase, squash my commits and fix merge conflicts along the way.

@emwalker I was a bit unsure about the whole default and new thing, in general I thought that a new constructor without parameters could be replaced with a call to default, however there also some use cases where I see that a new constructor makes sense semantically, like for arenas or buffers.

I've stumbled on this clippy lint where the fix section seems like a sane alternative and if everyone is ok with that I would change it to that style.

emwalker · 2023-11-27T23:41:05Z

I would do another rebase, squash my commits and fix merge conflicts along the way.

Judging from experience, your rebase might be easier if you squash your commits first so that you only have one or two commits to rebase off of latest main. But I'm sure you'll figure something out.

E.g.,

$ git rebase -i "dd684aa502924ed0e86a70a768308f70e4e2089c^" # interactive rebase, squash everything here

I've stumbled on this clippy lint where the fix section seems like a sane alternative and if everyone is ok with that I would change it to that style.

Feel free to change some of those ::default() calls back to ::new(). On the off chance you were thinking of updating the linting rules, I'd skip that for this PR.

jaytaph · 2023-11-29T21:37:45Z

@emwalker @Sharktheone @Kiyoshika CI is happy.. feel free to merge

Sharktheone

Only some small changes, I might have missed last time.

Sharktheone · 2023-11-29T21:56:16Z

src/render_tree/properties.rs

 impl Default for Rectangle {
    fn default() -> Self {
+        Self::new()
+    }


Couldn't you use #[derive(Default)] here because the default rectangle uses zeros for the new method?

I guess I could, I just wanted to be consistent with the other default implementations. A pro imo is that, if you change the new constructor to accept parameters, then you are forced to reason about what the default implementation should do, or you even need one. If we derive the trait, then this connection is lost and this could lead to surprises in scenarios where you called default instead of new.

Sharktheone · 2023-11-29T22:00:36Z

@emwalker @Sharktheone @Kiyoshika CI is happy.. feel free to merge

Yup, LGTM. I only found one small thing, but it wouldn't be problematic if it wouldn't be changed.

emwalker

@emwalker @Sharktheone @Kiyoshika CI is happy.. feel free to merge

We still need to rebase into a single commit. @jaytaph I think you still don't like using the "Squash and merge" button? Please advise. We want atomic commits that will make using git bisect unproblematic.

@koopa1338 can you rebase this PR into a single commit and force push the result back to your branch?

- use mod.rs instead of empty named module file - replace emtpy new constructor with defaulf implementations - sorting derives - replaced ElementClass::from_string method with From trait impl - rename `Buffer::to_string` to `try_to_string` to indicate possible failure - default impl for `TotalTestResutls` - make function arguments more readable/flexible - better usage of iterators and options, as well as minor improvements - remove iter calls in for loops - asserts, matches and if let bindings - must use attributes - strings and closures - debug and default impls - borrows in arguments - filter iterators - make use of `Self` keyword in impl blocks

koopa1338 · 2023-11-30T00:30:29Z

Sorry for that, didn't know that the squash and commit is disabled, squashed it all in a single commit, should be fine now.

emwalker

Congratulations, @koopa1338, on your first Gosub PR. Many thanks for the help.

…ion guidelines

koopa1338 marked this pull request as draft November 19, 2023 22:01

koopa1338 changed the title ~~Draft: Improving Code Idiomacy~~ Improving Code Idiomacy Nov 19, 2023

Kiyoshika reviewed Nov 20, 2023

View reviewed changes

koopa1338 marked this pull request as ready for review November 20, 2023 22:15

jaytaph requested review from emwalker, neuodev and Sharktheone November 22, 2023 20:30

jaytaph requested a review from Kiyoshika November 22, 2023 20:53

koopa1338 force-pushed the refactor/idiomatic-rust branch from 8d50acd to 7460aab Compare November 22, 2023 21:55

Sharktheone reviewed Nov 22, 2023

View reviewed changes

emwalker requested changes Nov 23, 2023

View reviewed changes

koopa1338 force-pushed the refactor/idiomatic-rust branch 3 times, most recently from 37584b2 to dfac1d8 Compare November 29, 2023 20:11

koopa1338 requested review from emwalker and Sharktheone November 29, 2023 20:14

Sharktheone requested changes Nov 29, 2023

View reviewed changes

Sharktheone mentioned this pull request Nov 29, 2023

Css3 parser tryout #286

Closed

emwalker requested changes Nov 29, 2023

View reviewed changes

koopa1338 force-pushed the refactor/idiomatic-rust branch from 4468e90 to def9332 Compare November 30, 2023 00:28

koopa1338 requested a review from emwalker November 30, 2023 00:30

emwalker approved these changes Nov 30, 2023

View reviewed changes

emwalker merged commit 45593c6 into gosub-io:main Nov 30, 2023
4 checks passed

koopa1338 deleted the refactor/idiomatic-rust branch November 30, 2023 08:29

koopa1338 mentioned this pull request Nov 30, 2023

Improve Node initialization #289

Closed

Kiyoshika mentioned this pull request Dec 1, 2023

Revert mod.rs files #296

Closed

Sharktheone added a commit to Sharktheone/gosub-engine that referenced this pull request Dec 14, 2023

revert mod.rs files introduced in gosub-io#268 according to contribut…

f135380

…ion guidelines

Sharktheone mentioned this pull request Dec 14, 2023

revert mod.rs files #316

Merged

Improving Code Idiomacy #268

Improving Code Idiomacy #268

Conversation

koopa1338 commented Nov 19, 2023

Changes Made

Sharktheone commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone commented Nov 20, 2023

koopa1338 commented Nov 20, 2023

jaytaph commented Nov 22, 2023

koopa1338 commented Nov 22, 2023

Kiyoshika commented Nov 22, 2023

Kiyoshika commented Nov 22, 2023

Sharktheone left a 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

koopa1338 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone commented Nov 22, 2023

emwalker left a comment • edited Loading

Choose a reason for hiding this comment

Sharktheone commented Nov 23, 2023

jaytaph commented Nov 23, 2023

Sharktheone commented Nov 23, 2023

emwalker commented Nov 23, 2023 • edited Loading

koopa1338 commented Nov 27, 2023

emwalker commented Nov 27, 2023 • edited Loading

jaytaph commented Nov 29, 2023

Sharktheone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone commented Nov 29, 2023

emwalker left a comment • edited Loading

Choose a reason for hiding this comment

koopa1338 commented Nov 30, 2023

emwalker left a comment

Choose a reason for hiding this comment

koopa1338 Nov 27, 2023 •

edited

Loading

emwalker left a comment •

edited

Loading

emwalker commented Nov 23, 2023 •

edited

Loading

emwalker commented Nov 27, 2023 •

edited

Loading

emwalker left a comment •

edited

Loading