IAU-ADES / ADES-Master

ADES implementation based on a master XML file
26 stars 7 forks source link

Files renamed to .py, tests added #18

Closed federicaspoto closed 12 months ago

federicaspoto commented 1 year ago

@stevechesley I just learned this new feature about "Draft pull request". It will allow me to write what I'm doing while I'm doing it and you should be able to see everything.

federicaspoto commented 1 year ago

@stevechesley @matthewjohnpayne I believe that this is complete.

matthewjohnpayne commented 1 year ago

Everything looks good, with the following exception:

I recommend changing test_mpc80coltoxml.py as described below, so that when pytest is executed, everything behaves in a standard manner:

Then when you run "pytest test_mpc80coltoxml.py" you should see ...

federicaspoto commented 1 year ago

@matthewjohnpayne thank you! I have modified the test.

matthewjohnpayne commented 1 year ago

I'm happy to merge But I'll leave that to you/Steve once Steve has had chance to review M

stevechesley commented 1 year ago

@federicaspoto I think this could be ready to merge, but I propose that we just keep working on this branch before merging. I see three things that could/should be done before the merge:

  1. Incorporate argparse into the scripts so that arguments handling would be more flexible and standardized, with automatic documentation.
  2. Adapt a traditional functional approach, rather than just passing the command line array as a sole argument. So you would call function(arg1, arg2) rather than function(["", "arg1", "arg2"]).
  3. implement tests for invocation as a function, in addition to the current tests for command line use.

I think items 1 & 2 are intertwined and would best be done together. For item 2, nobody outside of the MPC should have implemented these functions yet since this is still a development branch. So it seems a good moment to standardize/modernize the functional form before merging.

We can take care of all of these in fairly short order if you concur.

stevenstetzler commented 1 year ago

@federicaspoto I've implemented suggestion 1 + 2 from Steve's list and pushed the code in PR #23 so you can review it. If you give the go-ahead, I can merge that into this branch. And I can implement 3 from Steve's list if you approve. Thanks!

federicaspoto commented 1 year ago

@stevechesley @stevenstetzler I added the new tests, as we talked about.

stevechesley commented 12 months ago

@stevenstetzler Is the argparse ready to merge? Looks like it needs to first have the master merged into the branch. But not sure if it's ready yet. Feel free to go ahead with a pull request when it's ready.