Feat/add full date capabilities #34
Conversation
src/components/flac_tag.rs
Outdated
| fn remove_year(&mut self) { | ||
| self.remove("YEAR"); | ||
| self.remove("DATE"); | ||
| } |
There was a problem hiding this comment.
This is actually correct, as you cannot have a date without a year. It would be surprising behavior to have the DATE retained if you delete the year.
src/anytag.rs
Outdated
| pub date_released: Option<Timestamp>, | ||
| pub original_date_released: Option<Timestamp>, | ||
| pub date_recorded: Option<Timestamp>, |
There was a problem hiding this comment.
| pub date_released: Option<Timestamp>, | |
| pub original_date_released: Option<Timestamp>, | |
| pub date_recorded: Option<Timestamp>, | |
| pub date: Option<Timestamp>, |
ID3v2 is the only tag with a concept of original dates and release times. When going generic, there should only be recording date, as that is the only date supported by all formats.
src/traits.rs
Outdated
| fn original_date_released(&self) -> Option<Timestamp>; | ||
| fn set_original_date_released(&mut self, date: Timestamp); | ||
| fn remove_original_date_released(&mut self); | ||
|
|
||
| fn date_released(&self) -> Option<Timestamp>; | ||
| fn set_date_released(&mut self, date: Timestamp); | ||
| fn remove_date_released(&mut self); |
There was a problem hiding this comment.
| fn original_date_released(&self) -> Option<Timestamp>; | |
| fn set_original_date_released(&mut self, date: Timestamp); | |
| fn remove_original_date_released(&mut self); | |
| fn date_released(&self) -> Option<Timestamp>; | |
| fn set_date_released(&mut self, date: Timestamp); | |
| fn remove_date_released(&mut self); |
With the removal of the fields from AnyTag, these methods should be taken out as well.
There was a problem hiding this comment.
Ha, you beat me to it. Just removed them. Thanks!
| } | ||
| fn remove_year(&mut self) { | ||
| self.inner.remove_date_recorded(); |
There was a problem hiding this comment.
This should be good once this is added back. I missed this in the review. :)
|
@pinkforest Pinging so this is on your radar. Should be good to merge after the last comment is addressed. |
|
Just checking quick - Is this PR kosher - there seems to be open conversations ? Thx you both |
|
Yep, this is good. All comments were addressed. |
Hi! Thank you for making this crate!
I noticed that there didn't seem to be a way to get the full date from a tag, only a year i32. The tags across flac/id3/mp4 all seem to support a full date in some sort of way, but it doesn't seem to be standard across them. The id3 library seems to be the most specific with how it handles the dates, so for this PR I initially went with specifying the dates with the most specificity across the formats, rather than attempting to keep it at the lowest common denominator (like it seems you might be trying to do). I also just reused the id3::Timestamp, which you might not find desirable, but I could change that to something more standard, if you have something in mind.
Also, forgive my ignorance on the audio specs. I'm not very familiar with them, so if this all seems like an anti-pattern, please let me know.
Thanks!