cli
cli copied to clipboard
exercism
Reduce the returns in Dir to 2 and make it shorter
Did you close this on purpose?
Yes, I thought you didn't like it or something but I will reopen it if you want...
@RHL120 You closed it again, I am assuming for the same thought that it is not liked. We just may not be ready for it to come in yet. If you close it, then it is likely that this change may be forgotten about, regardless of its quality and if we like it.
(I know it can be irritating to have a PR open for a long time, seemingly ignored and neglected. But you took the time to write it, let us take the time to review it, where we can take the proper time it deserves.)
@RHL120 You closed it again, I am assuming for the same thought that it is not liked. We just may not be ready for it to come in yet. If you close it, then it is likely that this change may be forgotten about, regardless of its quality and if we like it.
(I know it can be irritating to have a PR open for a long time, seemingly ignored and neglected. But you took the time to write it, let us take the time to review it, where we can take the proper time it deserves.)
Ok i am sorry if it was annoying to have someone opening and closjng Prs. I will reopen it.
@RHL120 I appreciate your efforts in this PR. However, I am not convinced this refactor is A) a true refactor (tests confirming pre and post refactor functionality would be required to prove this) and B) that the modest readability improvement justifies the risk of changing this critical section of (what I think is) currently untested logic.
Ok great! What should I do?
@RHL120 I appreciate your efforts in this PR. However, I am not convinced this refactor is A) a true refactor (tests confirming pre and post refactor functionality would be required to prove this) and B) that the modest readability improvement justifies the risk of changing this critical section of (what I think is) currently untested logic.
Ok great! What should I do?
You could add one or more unit tests documenting and validating the existing functionality in a separate PR. Once that is merged, if the tests still pass with the refactor, we can reconsider this PR. To be candid I'm not sure if it's worth the effort, as the readability is improved in this PR, but I would call it a modest improvement and probably not worth the risk of the change. I'd defer to @ErikSchierboom if it is worth continuing to pursue this PR assuming tests were added. The tests on the other hand, those are definitely valuable and would be a welcome addition to the codebase.
I'd defer to @ErikSchierboom if it is worth continuing to pursue this PR assuming tests were added.
I'm not a Go programmer and don't have a natural affinity with how Go code should be formatted, so I don't think I can have a well-based opinion.
The tests on the other hand, those are definitely valuable and would be a welcome addition to the codebase.
Agreed. Tests would be very helpful.
Having looked at this PR, I'm not sure the refactoring actually improves readability much (which is subjective, I admit). Therefore, I'm closing this PR. Thanks for the PR!