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.

Sign up for GitHub

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

Jump to bottom

Adacs dev #412

Merged
merged 40 commits into from
Nov 26, 2020
Merged

Adacs dev #412

merged 40 commits into from
Nov 26, 2020

Conversation

shiblisaleheen
Copy link
Contributor

Changes related to fixing the existing test cases, disabling the old ones, fixing the npm version and update readme.

shiblisaleheen and others added 20 commits October 9, 2020 12:19
@shiblisaleheen
adding nvmrc to fix the version numbers
5fe0f4f
@shiblisaleheen
add .idea to gitignore
63941ba
@shiblisaleheen
add wayaround to fix npm start failure
43bcdbc
@dliptai
fix test paths (./pipeline/tests --> ./vast_pipeline/tests)
0a445b4
@shiblisaleheen
try to fix test
ca87f2a
@shiblisaleheen
Merge branch 'adacs_dev' of https://github.com/askap-vast/vast-pipeline…
7904bb0 
… into adacs_dev
@shiblisaleheen
fix config with new defaults
1490b63
@dliptai
fixed webserver test error
c803f6d 
use django default for STATICFILES_STORAGE setting during testing (as per the django documentation)
@shiblisaleheen
adding nvmrc to fix the version numbers
7f3de93
@shiblisaleheen
add .idea to gitignore
891c8e0
@shiblisaleheen
add wayaround to fix npm start failure
6f4121a
@shiblisaleheen
try to fix test
35ff104
@dliptai @shiblisaleheen
fix test paths (./pipeline/tests --> ./vast_pipeline/tests)
0bfa49d
@shiblisaleheen
fix config with new defaults
276c995
@dliptai @shiblisaleheen
fixed webserver test error
0431f7b 
use django default for STATICFILES_STORAGE setting during testing (as per the django documentation)
@shiblisaleheen
merge
0fc7970
@shiblisaleheen
fix duplicate files test
c85db85
@shiblisaleheen
fixed test cases
9100480
@shiblisaleheen
activate all tests
1c95a12
Add profile notes
bb5160e
@shiblisaleheen shiblisaleheen requested a review from srggrs November 9, 2020 23:20
github-actions[bot]
github-actions bot reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on your first PR: please assign the PR to a milestone, and to a reviewer if not auto-assigned. Add relevant labels if possible

srggrs
srggrs reviewed Nov 10, 2020
View reviewed changes
Copy link
Contributor

@srggrs srggrs left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! Looks good over all. Just few questions below:

  • waht is the .idea file
  • why we need the .nvmrc file

srggrs
srggrs requested changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing the association tests! They seems great! I will get Adam to review them as well, as he understand more the association logic. Overall I commented few things, just to make more conform with the Django guidelines...

@ellawang44
Copy link
Contributor

@srggrs @ajstewart I'm done with the changes in associate. Please review these tests.

ajstewart
ajstewart requested changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look fantastic, thank you!

I've look through all the logic of the tests and as far as I can see you've got it all correct for each test.

Just some minor things:

  1. CSV new line warnings - doesn't really matter but would be neater to get rid of them (unless this is to make them be read easier?).

  2. Function docstrings - recently we've been making sure that any new code has the correct docstrings implemented with the outputs and variables. It would be good if this could be followed for those functions which have variables or outputs (the format needs to be standardised in the code so you may see two different styles).

  3. Please update CHANGELOG.md with the changes made.

Update changelog
e8d4594 
Added new lines to csv, added more details to docstrings
@ellawang44
Copy link
Contributor

@ajstewart I made the requested changes, please review again and let me know if there's any other issues.

ajstewart
ajstewart reviewed Nov 25, 2020
View reviewed changes
@ajstewart
Copy link
Contributor

All looks good to me, just the conflict to sort out and then it should be good to go.

@ajstewart
Copy link
Contributor

Not sure if @srggrs wants another look, a tag in case.

@srggrs srggrs self-requested a review November 25, 2020 05:09
srggrs
srggrs approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @shiblisaleheen @ellawang44 ! LGTM!

@srggrs srggrs self-requested a review November 25, 2020 05:12
srggrs
srggrs requested changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @shiblisaleheen @ellawang44 ! LGTM!

just merge master and send us the request for review for approval

srggrs
srggrs previously approved these changes Nov 25, 2020
View reviewed changes
@ajstewart
Copy link
Contributor

Gah sorry I just merged one of the others! One more merge master and it will good, I won't touch it until this is done.

@shiblisaleheen
Copy link
Contributor Author

Not a problem, I just merged back master into adacs_dev @ajstewart

@ajstewart
Copy link
Contributor

Feel free to merge this!

@shiblisaleheen
Copy link
Contributor Author

I don't have permission to merge this :(

@ajstewart
Copy link
Contributor

I don't have permission to merge this :(

Whoops! I'll merge it now.

@ajstewart ajstewart merged commit fca4bcc into master Nov 26, 2020
@ajstewart ajstewart deleted the adacs_dev branch November 26, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellawang44 ellawang44 ellawang44 left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@ajstewart ajstewart ajstewart approved these changes

@srggrs srggrs Awaiting requested review from srggrs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shiblisaleheen @srggrs @ellawang44 @ajstewart @dliptai