search

edsu / pymarc

process MARC records from Python
http://python.org/pypi/pymarc
Other
251 stars 99 forks source link

Allow MARCReader to return custom Record objects #141

Closed herrboyer closed 4 years ago

herrboyer commented 4 years ago

Hi,

It would be nice to to make MARCReader return a custom Record object, instead of pymarc.Record (ie one with custom methods, etc.).

This PR adds a record_class property to MARCReader to make it easily changeable in a subclass.

edsu commented 4 years ago

This is an interesting idea, and I appreciate that you have added tests. But I'm actually kind of curious to know what it is you need to subclass pymarc.Record for, and whether it would be useful to others.

herrboyer commented 4 years ago

We handle issn records in marc21, we are refactoring our code right now by subclassing pymarc.Record and add all our custom method (.issn(), and other stuff specific to our needs). It would provide a lot of value for us if we where able to properly (meaning not overwriting the full MARCReader.__next__method) get our custom Record object when iterating a file.

edsu commented 4 years ago

Can you describe what "other stuff" entails? Enhanced ISSN logic seems like something that would be generally useful to the pymarc community.

Wooble commented 4 years ago

I'd typically prefer composition over inheritance for cases like this (reading the Record objects and wrapping them in a class with additional methods), or just functions that give the desired data when passed a Record.

That said I'm not particularly opposed to be able to inject a different class to build from the Reader.

herrboyer commented 4 years ago

@edsu : I'll make another pull request as soon as our refactor is tested, we'll gladly contribute back to this project.

It will essentially be a few « handy methods » as you call them on the README : shortcuts to get the issn number, alternate titles, transliterated titles, timestamps, etc. We will have some setter methods as well (but for now we focus on getting data).

Do you have any guidelines of what could be added to pymarc.Record ?

@Wooble : I think that's what I'm doing here on the Record side (we wrap pymarc.Record in an ISSNRecord with extra methods), our problem was : how to make MARCReader return this custom object ?

librarywebchic commented 4 years ago

To build on what was already said, I'm looking at making some extra methods which would be specific to handling field and structures in a MARC Holdings data. I'm wanting to read in MARCXML though.

herrboyer commented 4 years ago

Hi @edsu & @Wooble, any news for this PR ? I've just got a new use case for it : I need to change a few thing on the string representation of a record, something pretty specific to our needs that would probably not really be of any use to the pymarc community (and would break stuff if pymarc.Record.__str__ was to be changed).

edsu commented 4 years ago

I'm sorry for the delay @herrboyer -- would you be willing to look into what needs to be adjusted to fix the conflicts this PR has with master?

I'm still kind of lukewarm on allowing different classes to be added in. I would prefer for new functionality to be added to pymarc rather than letting people add things closed source. But I suppose it's not going to hurt that much.

herrboyer commented 4 years ago

Hi @edsu I now see how this could lead to an open door to closed source development around pymarc, fair point.

I think I will close this PR and start working with @petrus-v on our fork, experiment a bit and hopefully come with useful additions to pymarc's Reader & Record. Would that be OK for you ?

edsu commented 4 years ago

Sure, that's fine. Let me know if I can help.