paritytech / parity-scale-codec

Lightweight, efficient, binary serialization and deserialization codec
Apache License 2.0
257 stars 94 forks source link

Feature/refactor check repr #641

Closed fewForce closed 3 weeks ago

fewForce commented 1 month ago

This PR :

  1. Resolved conflicts arising from upgradation of dependency syn 1.98 to syn2
  2. Added find_meta_item_in_path function to enhance utils.rs by reducing code redundancy. The new implementation uses Rust's functional programming features for improved readability & conciseness
fewForce commented 3 weeks ago

what is it fixing actually?

@gui1117 It addresses Code Duplication It reduces the lines of code.

Explanation Following code is used at 3 different places in the same file with following 2 changes -

find_meta_item(field.attrs.iter(), |meta| {
        if let Meta::Path(ref path) = meta {
            if path.is_ident("compact") {
                let field_type = &field.ty;
                return Some(quote! {<#field_type as #crate_path::HasCompact>::Type});
            }
        }
gui1117 commented 3 weeks ago

If it is only for fixing the duplication of code, I personally think the new code is a bit less readable.

If some other people want this change I can re-open the PR.

fewForce commented 3 weeks ago

If it is only for fixing the duplication of code, I personally think the new code is a bit less readable.

If some other people want this change I can re-open the PR.

@gui1117

Code Readability depends upon the experience of a Developer. Experienced programmers would say that code should be concise & clear. To develop for Polkadot’s substrate codebase is not for the light of heart. Also, I believe removing inadequacies ( how so ever minor ) in code should be promoted by experienced engineers like yourself.

Code Duplication is a tangible metric. It makes code not look ugly but feels more like novice’s job. So many people have seen this code and would notice the duplication. Rather than trying to go through the process of making a PR, they choose to accept the lower quality code. Now, waiting for others to say something is probably condoning bad coding practices by parity.

gui1117 commented 3 weeks ago

I also think the usage of FnMut + Clone feels not so good.

I actually appreciate the effort you put into making a PR, thank you for having taking the time.

That said I don't think the PR itself is worth merging. (Every review takes time)

If you want to contribute I think there is issue to do in polkadot-sdk that can be great to fix.

fewForce commented 3 weeks ago

I also think the usage of FnMut + Clone feels not so good.

I actually appreciate the effort you put into making a PR, thank you for having taking the time.

That said I don't think the PR itself is worth merging. (Every review takes time)

If you want to contribute I think there is issue to do in polkadot-sdk that can be great to fix.

@gui1117

I believe there is no minimum bar for a PR that can be set to stop good code from merging.

Please let me know if there is a process to appeal or ask for a different reviewer with little bit more experienced & open mind who can understand more complex code.