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

Custom GA workflow #1899

Merged
merged 2 commits into from
May 2, 2018
Merged

Custom GA workflow #1899

merged 2 commits into from
May 2, 2018

Conversation

jamesvanmil
Copy link
Contributor

Closes #1767

Introduces a work-wise workflow for managing the collection of GA stats

@coveralls
Copy link

coveralls commented Apr 20, 2018

Coverage Status

Coverage decreased (-0.4%) to 95.869% when pulling 0618cf5 on maint/1767-ga into 1c59a0a on develop.

@crowesn crowesn self-assigned this Apr 23, 2018
@crowesn
Copy link
Contributor

crowesn commented Apr 24, 2018

👍 I think this looks sound - do (or can) we test this on QA?

@crowesn crowesn assigned scherztc and unassigned crowesn Apr 25, 2018
@scherztc scherztc removed their assignment Apr 26, 2018
@jamesvanmil jamesvanmil force-pushed the maint/1767-ga branch 5 times, most recently from 6ee2ce7 to 358b374 Compare May 1, 2018 12:47
@hortongn
Copy link
Member

hortongn commented May 1, 2018

@jamesvanmil I'm giving this a 👍and will merge in the morning. It's a great approach to fixing really broken code.

I do have some suggestions to keep mind when doing future issues or if we ever swing back around and refactor this code, but it's not worth blocking a merge right now.

The WorkAndFileIndex class should be refactored. If you have "and" in your class name, IMO that's a code smell that it should be split up into two separate classes (e.g. WorkIndex and FileIndex) or better yet, generalized into something like ObjectsIndex to handle works or files without using a bunch of duplicated methods. WorkIndex and FileIndex could inherit from ObjectsIndex if needed.

The rescue lines in CustomStatImporter are not well tested. I think you mentioned that.

@hortongn hortongn self-requested a review May 1, 2018 20:53
@jamesvanmil
Copy link
Contributor Author

👍

@hortongn hortongn merged commit 0f8d7b5 into develop May 2, 2018
@hortongn hortongn deleted the maint/1767-ga branch May 2, 2018 12:43
@hortongn hortongn self-assigned this May 28, 2018
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.

5 participants