[GitHub] rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV
rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV URL: https://github.com/apache/incubator-superset/pull/3705#issuecomment-341327403 I don't want to sound stubborn, however, I do not understand why these two lines of code would require to write a test for the whole part of the function (exporting in case that a cache is configured) which was entirely untested beforehand. The most relevant part of this functionality is not mine and the overhead of writing a test, especially the one of mocking a cache back end is way too much for my limited understanding of the code and the tiny contribution of this PR. Please don't get me wrong. I do appreciate everybody's work on this wonderful piece of software and I do recognize that my contribution is mostly negligible. However, this is all that I can muster up at the moment. In other words, I am afraid that I cannot provide the demanded test code. But it would be a bad decision to leave issue #3704 unresolved for this reason, as it is a quite noticeable one for MS Excel end users. My humble suggestion would be that the original author provides a test for the 'export from cache' functionality. @mistercrunch: I'm not proud of this, but I would kindly ask you to take notice of this issue raised by the automatic coverage tests. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV
rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV URL: https://github.com/apache/incubator-superset/pull/3705#issuecomment-338692866 Thinking about it: We use information that is at this point only present in the JSON stored in the cache. If the cache is not available ? which I suppose for any generalized test ?, then I really have no idea, how a test would have to be defined. The whole `csv` function has no tests defined, as I understand. Maybe other parts of this function should be tested first? @bkyryliuk I saw your name in the code; maybe you could give me a hand? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV
rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV URL: https://github.com/apache/incubator-superset/pull/3705#issuecomment-338639157 @xrmx: Spaces are fixed now. However, unfortunately I have no idea how to add a test, what needs to be tested and how to perform the test. I am willing to contribute, but I'm really not an experienced programmer. Adding just two simple lines should not create so much overhead, in my opinion. This is a bit frustrating, to be honest? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV
rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV URL: https://github.com/apache/incubator-superset/pull/3705#issuecomment-338639157 @xrmx: I did not introduce any whitespace. Could you please specify? Furthermore I have no idea how to add a test and what needs to be tested and how. I am willing to contribute, but I'm really not an experienced programmer. Adding just two simple lines should not create so much overhead, in my opinion. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV
rumbin commented on issue #3705: convert DATETIME columns when exporting from cache to CSV URL: https://github.com/apache/incubator-superset/pull/3705#issuecomment-338614185 What am I supposed to do when the **coverage/coveralls** test fails? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
