Comments (17)
There are methods in the code that do exactly the same things as built in Framework code. For instance in Tag.cs there is this method:
private static bool IsNullOrLikeEmpty (string value)
{
return value == null || value.Trim ().Length == 0;
}
Its not really a big issue but it is a bit odd as there are usages of string.IsNullOrEmpty() in the code as well. But then I think this has been left in due to most contributors just building on top of these. This is absolutely a hang over from the early mono days where not every CLI method had an implementation. But Mono is now so close to the CLI that it covers most of these things. For instance it was only in the last year or so that Mono moved to .NET HttpClient implementation.
In general the structure is ok but it does confuse Visual Studio / ReSharper. Folders == namespace in VS and since there as a vast number of File.cs types and Tags adding a new one in a folder is problematic. As a test I just opened one of the folders and added a class:
namespace TagLib.TagLib.Riff
{
class NewTag
{
}
}
This simple action rendered the entire solution into a non-compiling state with 665 CRC errors. Change this to:
namespace TagLib.Riff
{
class NewTag
{
}
}
And it compiles.
As for the test, I agree. But there wasn't a single use of the NUnit [SetUp] attribute which is exactly the method to use to not repeat unit test code as you say. There is no need to copy / paste. A series of unit similar tests go in a class with one method marked as [SetUp] (I usually name this method Before()) to setup the environment for the tests in the class. This [SetUp] method is run exactly once prior to nunit running any test in that class. There is also a [TearDown] that is run after every test in the class for any cleanup code.
In fact many of the tests in this project aren't even unit tests, they are integration tests since they reach out to system resources (the file system) but that's just semantics in many ways since the library is very file system centric through its nature.
from taglib-sharp.
Hi @jamsoft,
The Riff implementation uses the CombinedTag class. It means that whatever format supported by this class can contain multiple tag-format.
I think the good way to start is to add a BEXT directory which contains a File.cs and Tag.cs, inheriting from the TagLib.File
and TagLib.Tag
. The new tag type (BEXT) should be added to the Riff class the same way as Id3v2 or DivX. Looking at one of these format implementation should give a hint how to proceed.
You will have to add a self-checking test in tests/fixtures/TagLib.Tests.TaggingFormats, that basically proves that you can handle information on this new format properly.
Once this works, and if every test pass (see README) on how to run the tests),, you can issue a "Pull Request" to get this integrated.
Hope this short explanation gives you a kick start !
from taglib-sharp.
Excellent chap!! Much appreciated. I'll get to look at this maybe tomorrow as I've been wanted to get this one licked for a while now. Thanks again.
from taglib-sharp.
Actually, from looking at the code a bit more I'm slightly confused why I would need to create a new directory/ File.cs combination. Broadcast Wave files (BWF) are all RIFF/WAV files. And the BEXT/iXML is a standard chunk in RIFF files. All the Tags which I want to implement belong to the Riff file type. There are quite a few bits to add to the LIST tag for instance.
I'm heavily involved in processing lots of advanced audio files from many different applications and there is a lot missing from the tag-libsharp Riff class that I'm planning on implementing.
from taglib-sharp.
I'm also a bit confused about the testing methodology, there doesn't appear to be a single .WAV file in the \tests\samples directory.
from taglib-sharp.
I was also unable to run the unit tests due to a BadImageFormatException when loading GTK# I already had GTK# 2.12 installed as I do a lot of mono/Xamarin stuff.
Switching the unit test project to compile in x86 solved the issue. Has this been reported by others? Any reason why this isn't the default currently in the repo?
from taglib-sharp.
All this probably means that nobody worked on a dedicated wav-friendly implementation yet. That makes your contribution even more appreciated! Don't hesitate to create a (dummy) wav file for supporting your test (beware of size and copyright). Recoding 3 seconds of nothingness should make the trick for example. If this is really wav-specific, then you can create a FileFormats test instead of a TaggingFormats Test.
About Mono, I can't tell as I am on Windows...
And I am not too much aware of the BEXT format. So if it is, indeed, a Riff extension, just extend the Riff implementation (Tag/File...). Can not help you more right now, as I am on vacation.
from taglib-sharp.
No problem chap. I've made MANY tests and I have the core of this working now, reading and writing. The BEXT chunk is historical really, BEXT data is now formalised somewhat and included in the iXML spec.
I have a large pool of wav files at my disposal, some are very narly to deal with and I still have some issues with those and their various chunks to iron out.
One test file I have been working with has a LIST chunk that taglib# just cannot read properly so I'm still looking into that.
from taglib-sharp.
@Starwer You can close this ticket now, I'm pretty clear on the requirements.
from taglib-sharp.
from taglib-sharp.
I don't understand this. I don't have mono, and I run it successfully on Visual Studio 2015 by just following the indications in README file. It reports 0 error/warning.
from taglib-sharp.
To be honest there is a lot of formatting issues imho. The layout of the project file and namespacing doesn't follow standards. If you add a new tag class VS will add a class in a namespace like TagLib.TagLib.Riff.xxxxx. BOOM problems. This is all an overhang from early mono days and is completely understandable.
The coding style looks like Javascript and I have to fight with VS to maintain that layout (which I've given up doing actually).
But I do know that a lot of C# devs would be put off getting involved simply because of these formatting and structure issues.
I also think the lack of {} characters when scoping code is a nasty habbit. I know C# and the compiler don't mind but having explicit scopes in the code is a good idea in general.
The unit tests are laid out where they aren't really atomic, there are a lot of calls to utility methods which isn't always a problem but is a bit hard to follow at times which for a unit test is a bit of an anti-pattern in some ways.
from taglib-sharp.
Ok, I didn't realize there were such a strong coding-style requirement from the C# dev community. In comparisons to the projects I've been part of in many programming and scripting languages, I found that the code was looking pretty good, and well structured, especially when we think of how many voluntary contributors are involved. That's probably because I've seen too many ugly codes from people who had no idea on what maintainability could mean, or had no clue what OOP is good at. I have no strong opinion of what the ideal structure should be.
The unit tests are laid out where they aren't really atomic, there are a lot of calls to utility methods which isn't always a problem but is a bit hard to follow at times which for a unit test is a bit of an anti-pattern in some ways.
On the other hand, I have a strong opinion on that one. Having an atomic code is nice, but I'm quite happy that we break that rule for a way more important one: keep the code factorized. If you copy/paste something three times, you're doing it wrong. Or another one: "Three times or more, do it with a for". Back in the 90's, that was indeed a problem to have the code scattered on different files and functions. But VS is so good to bring you to where the function is defined ("Go to definition") that I think we now would be fools not to leverage this. If Microsoft did one good thing, it is the C#+VS combo.
from taglib-sharp.
IsNullOrLikeEmpty(" ")
will return true, whereas string.IsNullOrEmpty(" ")
will return false. In that sense, these are not exactly equivalent.
As for the test, I agree. But there wasn't a single use of the NUnit [SetUp] attribute (...) There is also a [TearDown] that is run after every test in the class for any cleanup code.
Nice ! I didn't know about this feature. I'll try to use it now if I create a new class.
from taglib-sharp.
Sorry, I meant:
return string.IsNullOrWhiteSpace(value);
Also a note of warning. The guys that wrote NUnit went on to create xUnit as they didn't like the [SetUp] and [TearDown] constructs as it meant tests weren't atomic and this lead to bad practice.
https://xunit.github.io/docs/comparisons.html
Note 2: The xUnit.net team feels that per-test setup and teardown creates difficult-to-follow and debug testing code, often causing unnecessary code to run before every single test is run. For more information, see http://jamesnewkirk.typepad.com/posts/2007/09/why-you-should-.html.
Have a read of the link there. The test code in taglib# is too difficult to follow. Test code isn't production code and copy and pasting some code between tests as detailed in that blog post is not an affront to the DRY principal. Anything in [SetUp] or [TearDown] should be the absolute minimum of code. Anything relevant to the actual test being run should be in the body of the test method itself.
The last major commercial project I worked on had over 7,000 tests just for one of its assemblies, setup and tear down was useful in that case but needs careful management or it actually starts to get in the way.
from taglib-sharp.
@jamsoft : Interesting reading. So eventually, no, I won't use SetUp
and TearDown
.
So for factorizing, the good old function/method is the way to go.
What I don't understand, is why developers seems to think that factorizing in the production code is important, and don't complain about lake of readability, whereas they seems to think that boiler plate and redundant code is preferable in tests. Do they think factorizing is only useful for code size optimization ?
Well I still think factoring is mainly a maintainability issue.
To be more concrete, how would you improve this code ?tests\fixtures\taglib.tests.fileformats\standardtests.cs
This class aims at testing the minimal set of features that every Video or Audio format should support. Different test fixtures call the same function with different file formats. This proves that TagLib# does its job in abstracting the different tagging formats. This was a great help for my PR #71 to add new tags properties to TagLib#. Just adding checks in this class I was actually spotting, fixing and proving the property would actually work seamlessly for 17 different formats.
How did I find out the 17 tests ? Right-click on WriteStandardTags
> Find all references
! That's it !
So yes, it was merely disturbing 1 min for readability, but it made me gain an hour later on #71. And in my opinion, this is the only proper proof that TagLib# can execute the same tagging scheme, regardless on the internal specificities of each format.
from taglib-sharp.
@jamsoft @Starwer @tcHylmrX I'd actually be really happy to see the code base cleaned up, but very carefully. Ie, don't turn resharper on and fix everything in one PR. That just isn't consumable :)
However, having said that... If someone wants to start going through and slowly cleaning things up in small PRs I'd be more than happy (along with others I'm sure) to review and merge them. I'd suggest we start with things like replacing code with stuff provided by the framework.
We should probably start a new issue around the code structure though. I've wondered about the File.cs thing as well. I think it warranty an issue and discussion.
I wouldn't be sad either if the unit tests were cleaned up too :)
As for a wav file. There might have been one. We used to have a git module with large files, but due to gitorious closing down and not pulling the repo from there in time we lost a directory of test samples :(
from taglib-sharp.
Related Issues (20)
- Wrong AudioBitrate at some ogg Files
- NestPlayer
- Access To The Path is Denied
- Creating tag RIFF tags in WAV file
- Id3v2 Tag Does Not Support Podcast Frames
- Error reading tag from Photomechanic
- How do I get the dimensions (height and width) of pictures HOT 2
- Impossible read/write tag software HOT 1
- Files not writeable by Taglibsharp but writeable by other tools HOT 1
- ID3v2 WXXX frame is empty HOT 9
- CombinedTag.IsEmpty returns true if any child is empty rather than if every child is empty
- Save Taging destroys highres wav
- PNG tags not being added HOT 1
- TagLibSharp Corrupting MP4\M4V files when updating Tag.Title HOT 1
- Support for classical music tags introduced in iTunes 12.5
- Cannot set tags for .jpg files HOT 1
- File Not Writeable message when setting data for JPG file
- Release type and Release status showing as null
- Finding the video Codec ID in an MKV file
- [BUG] Corrupted UnknownBox data
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from taglib-sharp.