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

Export from_str_header and csv_heade macros #22

Merged
merged 10 commits into from
Jun 17, 2024

Conversation

tobiasfunke1
Copy link

Hi,

another small improvement would be to export the two macros from_str_header and csv_header.

Then someone can easily use them to create custom headers like this:

use ezk_sip_types::{csv_header, Name};
use bytesstr::BytesStr;

csv_header! {
    /// P-Access-Network-Info header
    PAccessNetworkInfo,
    BytesStr,
    Name::custom("P-Access-Network-Info", &["p-access-network-info"])
} 

What do you think about this?

@kbalt
Copy link
Owner

kbalt commented Jun 6, 2024

Hey,

Before these macros can be made public we must ensure they don't use any relative import paths. The csv_header! macro uses ConstNamed Name HeaderParse ParseCtx ExtendValues PrintCtx OneOrMore etc. from the current module, requiring a bunch of imports.

The macros would need to be changed to address this. e.g. every Name is changed to $crate::Name, ConstNamed to $crate::header::ConstNamed etc.

@tobiasfunke1 tobiasfunke1 marked this pull request as draft June 8, 2024 08:00
@tobiasfunke1
Copy link
Author

Thanks for your answer! That's right, I missed that. I'll try to make the changes and then you can look over it again?

@tobiasfunke1
Copy link
Author

I think that fits now. What do you think? @kbalt

@tobiasfunke1 tobiasfunke1 marked this pull request as ready for review June 8, 2024 11:46
@kbalt
Copy link
Owner

kbalt commented Jun 12, 2024

The macro changes look good, but you've introduced a lot of formatting changes to imports and where clauses which don't seem related to this topic, do you mind reverting them and then squashing your commits?

@tobiasfunke1
Copy link
Author

I removed the formatting, Should be ready now, please check again. :)

@kbalt kbalt merged commit d34440c into kbalt:main Jun 17, 2024
1 check passed
kbalt added a commit that referenced this pull request Jun 17, 2024
@tobiasfunke1
Copy link
Author

Thanks!

@tobiasfunke1
Copy link
Author

Are you planning to create a new release which includes the changes from this PR, #21 and #20?

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.

None yet

2 participants