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

adds search functionality for Artist, Release, and Recording entities #4

Merged
merged 7 commits into from
Nov 9, 2013

Conversation

alexanderscott
Copy link
Contributor

Follows your entity-style conventions and adds search functions which take a search string and optional search filters. Also updated the README with examples and started some initial work on unit tests using mocha.

@maxkueng
Copy link
Owner

maxkueng commented Nov 8, 2013

Hey Alex! So awesome!! Thanks a lot for your contribution. That's quite a chunk of functionality.

Thanks for adding tests. I ran them and they're all green. I've wanted to add tests for a long time but never really got to it. I'll try to add some more in the future and make better checks.
If you want, you can add a Makefile (like below) and add mocha to the devDependencies in the package.json. Or I can do it later myself.

test:
    @./node_modules/.bin/mocha \
        --ui bdd \
        --reporter spec

.PHONY: test

The project uses a single tab instead of 4 spaces throughout the code. If you could change this quickly I will merge your code immediately and publish a new version to npm. I can fix the indentation myself but you would probably lose credits for your code in git blame.

How was your overall satisfaction with the code? This library is part of my first node project ever and I haven't touched it much ever since. Did things make sense for you? There's probably a bit more abstraction and wrapping than there should be for a simple API client.

Thanks again
Max

@alexanderscott
Copy link
Contributor Author

No problem Max! This is a cool project for sure - I've been using in some of my side projects and thought I'd help the cause. Botched commit 0576c67 retabbing to 4-spaced tabs instead of 2, but reverted and think it should be fixed with 864ed1a. Let me know if that meets the standard (my vim does funny things with displaying spaces/tabs).

Overall quite satisfied with the code. The abstraction and wrapping actually made it really easy to add the search functionality, since the convention and objects are already well-defined. The entity prototyping should also make for pretty clean object-specific mocha tests. I've only included some basic tests to get things started - would like to expand upon these in the future.

I do think that the lib is a little bloated though at ~1200 lines, and that the entities can be externalized away from the core mb functions. Would be nice to maintain some sort of TODO list with future plans (like adding tests), as well as adding an automated build and status indicator from https://travis-ci.org/ . Just my 2 cents.

-- Alex

maxkueng added a commit that referenced this pull request Nov 9, 2013
Adds search functionality for Artist, Release, and Recording entities
@maxkueng maxkueng merged commit 725fd87 into maxkueng:master Nov 9, 2013
@maxkueng
Copy link
Owner

maxkueng commented Nov 9, 2013

Looks great!

I have published version 0.2.0 to npm, added the Travis-CI hook and everything. A TODO list can be maintained in GitHub issues. I've just added issue #5 as a reminder to write better tests.

I'm glad you like the class abstractions and you are totally right about the library being bloated. It contains a lot of repetitive code and could probably very easily be reduced to half the size or less. MusicBrainz is working on a JSON Web Service (currently beta) which would allow to completely ditch the whole XML to JS stuff.

As far as I know there isn't really another MusicBrainz library for Node. So this library deserves to be properly done. Maybe a rewrite would be appropriate some time. The caching feature is also too rudimentary and doesn't work well with MB's linked entities.

If you are serious about wanting to make the library better I can give you write access to the repo directly. Of course that doesn't mean you have to do anything. Just let me know if you feel like. My email is in the package.json.

Thanks again

Max

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.

2 participants