-
Notifications
You must be signed in to change notification settings - Fork 60
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
Straydogstudio master #11
base: master
Are you sure you want to change the base?
Conversation
Looks great. I would not try to extract version checking into one place. In the runtime code it is only in one place. And from what I know it is not a problem to repeat code in tests, especially if it makes things clearer. KISS doesn't always apply. So my vote is to leave the version checking as is. Are you wanting the extra tests before you merge? |
Less about KISS, more about SRP. That method is extremely busy. I'll extract the record to row data conversions and label generation and toss in some specs for revue later tonight. |
Sounds good. Let me know if you want any help. |
@straydogstudio any news about PR? |
pleeeease 👍 |
@jhcasado @ricardodovalle I think we are waiting for @randym or another (e.g. myself) to finish the TODO list above. In the meantime, you can use my branch just to survive: https://github.com/straydogstudio/acts_as_xlsx It is not as robust as this pull request, but it works for now. |
@straydogstudio , I've been using your branch in production already. Thank you very much. 👍 |
Yeah, works like a charm, thx @straydogstudio 👏 |
@randym Have you looked into this issue since last year? |
TODO
[] Confirm docs/version bump
[] Manual testing
[] merge and ship