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

add more lifetime/generics support for o2o #17

Merged
merged 6 commits into from
Aug 30, 2024

Conversation

vnghia
Copy link
Contributor

@vnghia vnghia commented Aug 28, 2024

This is my attempt for #16.

@vnghia vnghia marked this pull request as ready for review August 28, 2024 13:56
@vnghia
Copy link
Contributor Author

vnghia commented Aug 28, 2024

@Artem-Romanenia please take a look at this

@Artem-Romanenia
Copy link
Owner

Hi, thanks a lot for a PR! I will look into it as soon as I have a chance, maybe today, definitely before weekend.

@vnghia
Copy link
Contributor Author

vnghia commented Aug 28, 2024

Hey it seems that you dont run cargo format and cargo clippy. I would like to propose another PR to do that. Is that good for you ? After this PR obviously.

@Artem-Romanenia
Copy link
Owner

Hi, thanks again for a contribution! I have a couple of notes though.

Generally, if there are Entity and EntityDto, I'm trying to make it so that o2o macro can be applied on any of them, for whatever conversion you may want. In this case, you've covered this scenario:

use o2o::o2o;

pub struct Entity {
    pub some_a: String,
    pub some_b: String,
}

#[derive(o2o)]
#[from_ref(Entity)]
pub struct EntityDto<'a, 'b> {
    #[from(~.as_str())]
    pub some_a: &'a str,
    #[from(~.as_str())]
    pub some_b: &'b str,
}

But I would also like to support something like this:

use o2o::o2o;

#[derive(o2o)]
#[ref_into(EntityDto<'a, 'b>)]
pub struct Entity {
    #[into(~.as_str())]
    pub some_a: String,
    #[into(~.as_str())]
    pub some_b: String,
}

pub struct EntityDto<'a, 'b> {    
    pub some_a: &'a str,
    pub some_b: &'b str,
}

Other than that, I would only ask you to add some tests in o2o-tests package (my tests are mess, so you just can loosely imitate what's going on there 😄). And also, when editing readme, I use "Markdown All in One" extension in VS Code, which syncronizes table of content. You can try using that or something similar, or I will just make some final touches to readme myself in the end, if you don't want to bother with this 😄

@vnghia
Copy link
Contributor Author

vnghia commented Aug 30, 2024

Hi, could you merge this PR first and run rustfmt and I will send the new PR to support that case ? Because to support this case I will have to do a lot of changes but everytime I press Ctrl+S, my vscode will auto format the file which introduces unrelated changes and make it hard for reviewing 😅

@Artem-Romanenia
Copy link
Owner

So, the problem here is that default formatting sometimes goes against my personal preferences, however unconventional they are 😄 (mainly, I often value vertical compactness over horizontal, since mostly I do rust on 34'' widescreen and I have no problems with long lines). But here's an idea. I've created features/lifetimes branch. Please edit this PR or create a new one to merge your changes there instead of main. I will accept it and do rustfmt (is it the same thing as cargo fmt?). Then later I can figure out how to configure custom formatting rules specific to this project and reformat again.

@vnghia
Copy link
Contributor Author

vnghia commented Aug 30, 2024

There are a lot of thing you can config with rustfmt or cargo fmt (I think it is the same). You can see all the config here https://github.com/rust-lang/rustfmt/blob/master/Configurations.md and I think you will find mainly max_width and comment_with and use_small_heuristics useful. In your case I think you can just put these lines inside rustfmt.toml, run rustfmt and call it a day

max_width = 400
newline_style = "Unix"
use_small_heuristics = "Max"

@Artem-Romanenia Artem-Romanenia merged commit 15c5980 into Artem-Romanenia:main Aug 30, 2024
1 check passed
@Artem-Romanenia
Copy link
Owner

Ok, I guess no harm if I do it as you've originally suggested. But I'll do the configuration later, as I currently don't have access to my regular setup.

@Artem-Romanenia
Copy link
Owner

I've just added a commit with formatting to main, you should be good to go (hopefully 😄)

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.

2 participants