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 Tag::date_raw to get date string before Timestamp conversion #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mnpqraven
Copy link

Right now date() will always attempt a conversion to Timestamp, and if the date format is not correct then not all fields will be returned. I think this can be a good escape hatch for getting the original date and parse to one's own format if needed.

@mnpqraven mnpqraven changed the title Add Tag::date_raw to get date string before Timepstamp conversion Add Tag::date_raw to get date string before Timestamp conversion Apr 28, 2024
@Serial-ATA
Copy link
Contributor

@pinkforest This looks fine.

@mnpqraven out of curiosity, what format are your dates in?

@mnpqraven
Copy link
Author

@pinkforest This looks fine.

@mnpqraven out of curiosity, what format are your dates in?

@Serial-ATA they are mostly in 'YYYY.MM.DD' so most of the parsing would fail or only the year is recognized and the days and months are 'None'

@Serial-ATA
Copy link
Contributor

Is that by choice, or is there some software out there writing dates like that? Both ID3v2 and Vorbis Comments are specified to have the yyyy-MM-ddTHH:mm:ss format.

@pinkforest
Copy link
Collaborator

Thanks you both - Could we by any chance put these behind something like raw opt-in non-default feature ?

I suspect exposing raw by-default would be hazardous as people may expect that it's well formed but it may have been injected with hazards where they must do their own sanitisation and payload checks for non-standard dates.

Should the date parsing be opportunistic conditional e.g. to handle YYYY.MM.DD ? And where some fields are empty etc.

Then also the problem with that is US/Rest date divide YYYY.DD.MM vs YYYY.MM.DD etc. big headache handling those yeah

@Serial-ATA
Copy link
Contributor

I think just having a doc comment explaining that the output could very well not be spec-compliant would be enough. In 99% of cases Tag::date() will give you the correct output, since the format of the dates is specified to follow ISO 8601.

@pinkforest
Copy link
Collaborator

Ok if that is the case and we worry about ergonomic overs spec complianc e-

Then why not change so it gets properly used and handled via enum-data:

pub enum TimestampTag {
   /// [`id3::Timestamp`]
   Id3(id3::Timestamp)
   /// Unknown
   Unknown(String)
}
    fn set_date(&mut self, date: TimestampTag) {
    }

That ensures people are aware of it and handle it to some level.

@Serial-ATA
Copy link
Contributor

That might be nicer, @mnpqraven would you be interested in implementing it that way?

- split date tag to enum
@mnpqraven
Copy link
Author

Then why not change so it gets properly used and handled via enum-data:

pub enum TimestampTag {
   /// [`id3::Timestamp`]
   Id3(id3::Timestamp)
   /// Unknown
   Unknown(String)
}

I've added the raw-date feature that includes the function and the Unknown variant of the enum, so now default users will only see Id3.

    fn set_date(&mut self, date: TimestampTag) {
    }

That ensures people are aware of it and handle it to some level.

Would changing from Timestamp to TimestampTag be a breaking change or major change? Right now I've update set_date to be using the above but i'm wondering if there's a way we can avoid changing the api

@pinkforest
Copy link
Collaborator

pinkforest commented May 5, 2024

Would changing from Timestamp to TimestampTag be a breaking change

Yes but it's okay as we can bump and it will be improve the API. Alternatively we can mark old functions #[deprecated] and add another that returns the enum but I don't think it's worth the hassle given we want people to handle special cases.

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.

3 participants