search

mytardis / mytardis-app-atom

MyTardis app for Atom consumption.
4 stars 10 forks source link

Merge of Tjdett and Microtardis branches #4

Open stevage opened 12 years ago

stevage commented 12 years ago

I've more or less tested this and it more or less works.

tjdett commented 12 years ago

@steveandroulakis I found a number of cases in this pull-request where old MyTardis functionality is referenced.

Could you please take a look and see if there are any additional issues.

steveandroulakis commented 12 years ago

@tjdett Aside from testing the functionality, I'm admittedly a bit stuck for time while I work on 3.0 (and do my 'actual' job at Monash) -- could you please highlight some of the old parts for me and I'll follow it up.

I'll be much free-er once the 3.0 code list is done (closer than you may think!). Thanks.

tjdett commented 12 years ago

@steveandroulakis I'm not an active maintainer for this app, so the following comments are merely suggestions. The final call is yours for this and all subsequent pull-requests. You're in a better position to evaluate the long-term interests of the MyTardis project when performing the merge....

As mentioned in comments, it's doubtful this is necessary: https://github.com/mytardis/mytardis-app-atom/pull/4/files#L1R32

time_util.py is probably unnecessary duplication - it should at least be checked against core util.py to see if it's compatible before it's merged in.

make_local_copy needs to be removed entirely: https://github.com/mytardis/mytardis-app-atom/pull/4/files#L3R30

Personally, I believe pull-requests incorporating new features should also include tests for those features (even if they're not particularly comprehensive). Without them, there's no guarantee that any of those features will stay working. Not having them also makes refactoring difficult (and as noted in the comments, I think there's plenty of potential for that).

steveandroulakis commented 12 years ago

@tjdett Hi Tim,

Thanks for the prompt reply, even though there's no requirement for you to do so. Much appreciated..

Steve

crawley commented 12 years ago

I'm coming in late, but I'd like support Tim (and the MyTardis submission guidelines) in saying that pull requests should include unit tests for new and modified functionality. This is particularly important for me where I'm tracking HEAD and I don't have resources for repeated user / acceptance testing.