Kixunil / struct_deser

Simple (de)serialization of structs
5 stars 0 forks source link

Fork of this crate #2

Closed xoac closed 4 years ago

xoac commented 4 years ago

I was inspired by this crate to create similar one - generally to learn using derive. You can check it out endian_codec

Kixunil commented 4 years ago

I'm glad that you found it helpful, and quite surprised that you find it not "clean and Rusty" - I do care about that. Could you point me at what exactly you found unclean? I'd prefer not to create too many similar crates doing the same thing, so could we join forces?

Note that I'm already using my crate in production, so I will have requirements on it working.

xoac commented 4 years ago

This all are my personal opinions:

  1. Crate name is misleading. Deser suggest this is de-serialization (only) and it's do serialization but I don't feel saving bytes in special order is serialization (but here I am not sure about serialization definition). When I think about serialization I mean you can serialize every struct. Serialize Vec or other types is out of scope for this library (be design).
  2. Assume you work with sb that don't know what endianness is.
    #[derive(Debug, StructDeser, A, Lot, Of, Different, Derive, Trait, That, You, Dont, Know, From, Whitch, One, Be, Attr, Is )]
    struct A {
    #[be] // What this mean? how to google it? 
    a: u32
    }
  3. #[derive(StructDeser)] don't allow split on to_bytes and from_bytes. For example similar to AsyncRead and AsyncWrite or Serialize / Deserialize
  4. When you work with different endianness (one struct is le another be and a third is mixed-endian) and you see code like this:
    A::from_bytes(&bytes); // is this a little-endian or big or maybe mixed?

I don't fell endian_codec is also correct name for this type of crate but I couldn't find out better name. Maybe endian_packed would be better.

xoac commented 4 years ago

I am not sure 4 is correct but in 0.1 you can't use A::from_bytes::(&bytes) since StructDeser derive don't provide this.

Kixunil commented 4 years ago
  1. I completely agree that the name isn't great. I was thinking at the time of creation, but couldn't come up with anything else.
  2. Ah, I see. I didn't want the attribute to be too long, but you have a good point.
  3. I was considering that but opted for simpler solution due to time pressure. Splitting it is definitely better.
  4. Hmm, that was kinda the point that you don't have to think about it too much and that it always does the right thing. For instance, the endianess isn't always mixed. The main idea behind my crate was to replace unsafe casting, but I wouln't call it cast either. Is there a better name?

Overall it looks like your crate is better. I will consider deprecating mine in favor of yours, but I have some much more important things to do right now.

wucke13 commented 4 years ago

Hi, I'm currently looking up smol and simple crates for packing and unpacking Structs. What is the case with this and the fork? I think it would help to have less but therefore maintained crates for this. So, is deprecating still an option?

I'm somewhat baffled that it took me more than an hour to just find all viable options for a small, maintained, safe struct packing/unpacking/serialisation/whatever lib that lets me directly control the bytes in the output (opposed to, let's say bincode).

Kixunil commented 4 years ago

I guess you got the impression that this crate is unmaintained due to lack of commits over a long period of time. I understand that and think it might be misleading way of assessing maintenance.

The crate works well for me and up until this point I got zero feedback on what to improve. That could mean:

I'm a bit sad there wasn't a tighter cooperation from @xoac, but he has all the right to make his own crate and I'm glad he made something that looks better. I updated the README with this information. I'm keeping this crate and repository available, but encourage people to use endian_codec instead.

Kixunil commented 4 years ago

So as a final step I decided to properly document every change that I'd like to see in my crate, if it's ever revived. This allows any contributor who can't use endian_codec to know what to do. I would merge good PRs based on the issues opened in the repository by me even if I don't have much time to do coding. But preferably, the contributions should be done to endian_codec to not waste the time.

I consider this issue resolved now, but let me know if you have any ideas about what else could be done about it.

xoac commented 4 years ago

@Kixunil I am open for cooperation. All crates are has major version set to 0. So deprecating crate is not a problem especially if there will be replacement.

wucke13 commented 4 years ago

First of, @Kixunil: Thank your for the excellent answer, you are, indeed, right regarding my perception. I too think it would be nicer to have two competent people providing one well working crate. I would love to see your efforts combined in one crate, but that's up to you.

@xoac Just to put it: IMHO the name of your crate is quite misleading. Endianness is just something which one has to think about while packing and unpacking structs. In my perception, the endianess handling is a detail, and packing/unpacking structs is the main issue that your crate solves. The question of endianness arises from its own. I would never imagine your crate to do something actually related to struct packing/unpacking just from the name. When I first read about it, I was rather confused as to why you call it codec. I do not perceive the order of bytes in integer to be a sufficient transformation of data for it to be actually a encoding/decoding process, as it may even happen that the data is not at all changed (if endianness of machine matches that of the format specified). I'm aware that technical this can be considered a codec, yet probably more people who hear about codec are more likely to think of advanced codes, like media codecs.

xoac commented 4 years ago

@wucke13 fell free to request better name. I am not native and I tried my best.

I don't think endian is misleading in this name. Since I am working with device that send me a raw bytes (C struct with #pragma pack(1) as little endianess) - and I want to write program that can read correctly this bytes no matter for what architecture my app is compiled. (So this crate allow you to abstract endianness).

codec is used for example by tokio-util It do a simple tasks:

There is no information about it should be complicated transformation or not. This is way I have chosen endian_codec

wucke13 commented 4 years ago

What about struct_codec and just having the statement that endianness is to be in full control of the programmer and can not be ignored in the README?

Abstracting endianness should be in any similar lib - always, because otherwise you would end up with a set of different codecs for different machines generated from the same code which obviously is bad. I absolutely support your claim that it is important, I just wouldn't put it in the name for the same reason that I prefer 'car' over 'wheel-car'.

The argument via Tokio is good, indeed.

xoac commented 4 years ago

@wucke13 In most cases abstacting endianess is hidden in my crate is not:

use endian_codec::{PackedSize, EncodeLE, DecodeLE};
// If you look at this structure without checking the documentation, you know it works with
// little-endian notation
#[derive(PackedSize, EncodeLE, DecodeLE)]
struct Version {
  major: u16,
  minor: u16,
  patch: u16
}

EncodeLE -- is from EncodeLittleEndian end dealing with endianess is main functionality. The goal of this crate is not serialize or deserialize (use bincode if you need this and don't care order of bytes) - it's to work with endianess and make it readable and easy to document.

struct_codec - endian_codec can be used with enum just derive will not work for it. I think struct_codec is also misleading since it suggest you can decode / encode struct with some Encode or Decoder. It's not true -- you can only encode / decode with endianess it's not general purpose.

If you want to discuss rename of endian_codec use this issue. I think sending notification to owner of struct_deser is not necessary.

Kixunil commented 4 years ago

You both raised some good points. I'm actually interested in the name. :) But yes, let's move over there. FWIW, I'd love to see an implementation for enums (using a tag), I even wanted to implement it here, just didn't have the time.

Kixunil commented 4 years ago

One last thing you two could help me with: I decided to write a general statement explaining how I maintain my stuff and plan to attach it to all my non-dead projects. It'd be nice to have some feedback on whether the statement is understandable and if it should contain more information that'd be needed for people considering use of my code.

https://gist.github.com/Kixunil/f82ec29df925f7cf49884d207d07e904

Thank you in advance!

xoac commented 4 years ago

I use maintenance label in Cargo.toml. example

# Maintenance: `status` is required. Available options are:
# - `actively-developed`: New features are being added and bugs are being fixed.
# - `passively-maintained`: There are no plans for new features, but the maintainer intends to
#   respond to issues that get filed.
# - `as-is`: The crate is feature complete, the maintainer does not intend to continue working on
#   it or providing support, but it works for the purposes it was designed for.
# - `experimental`: The author wants to share it with the community but is not intending to meet
#   anyone's particular use case.
# - `looking-for-maintainer`: The current maintainer would like to transfer the crate to someone
#   else.
# - `deprecated`: The maintainer does not recommend using this crate (the description of the crate
#   can describe why, there could be a better solution available or there could be problems with
#   the crate that the author does not want to fix).
# - `none`: Displays no badge on crates.io, since the maintainer has not chosen to specify
#   their intentions, potential crate users will need to investigate on their own.
maintenance = { status = "..." }

source

But adding additional note to a README.md is great option since a lot of people claim that labels are useless. I would love to see that note for every label in my template.

Kixunil commented 4 years ago

Hmm, I tried the maintenance label before and it didn't seem to work. I got some warnings. Maybe I was doing something wrong? If it works, it pretty much means many of my crates are passively-maintained.

wucke13 commented 4 years ago

@Kixunil Disclaimer: I'm not a native speaker.

please make sure to ask about a change you'd like before proceeding [if any].

I would not instruct people to ask for change unconditionally.

It's likely I will reply an cooperate.

This doesn't make a whole lot sense to me, grammatically. How about this:

[I'm likely to respond to your issues fast]

Other than that, these two stuck out to me:

I will make sure to at [...][least] [....][respond]

for you to do the chan[...]ges you need [yourself].