Skip tilde files when importing #2413
Conversation
|
This makes a ton of sense to me. @singingwolfboy can you or a Studio team member take a look? This is super tiny. |
|
@ichuang / @carsongee again this will need rebasing to pick up test infrastructure changes. It will also need test coverage before the Studio team can look at it. |
|
Is it possible to have a test for this change? |
|
@ichuang I wrote some tests for your changes, because we care about test coverage and we didn't have the infrastructure in place to easily test these files. Normally, writing tests should be handled by the person making the pull request, but I wanted to make an exception for this. Can you verify that I'm correctly testing the intent of your changes? |
|
Thanks, @singingwolfboy ! LGTM |
| files that end in a tilde (~) at the end of the list of results. | ||
| """ | ||
| result = glob(path) | ||
| with_tildes = [f for f in result if f.endswith("~")] |
There was a problem hiding this comment.
would it be worth asserting with_tildes is non-empty? Otherwise we may not notice if the test directory suddenly doesn't have the tilde files.
There was a problem hiding this comment.
I'd rather not do that here -- this is just a slight tweak to how the glob function works. (glob returns items in an arbitrary order, but this problem only occurs when files that end with tilde appear after items that don't end with tilde, so this just ensures that we're testing the correct scenario.) I could test that we have tilde files in the test directory, but that should be something handled by the test fixture -- if I did that, I would be testing the test, not testing the code.
However, I'll add a README to the test fixture directory, explaining that it's intentional for us to have tilde files committed to the repository here. That should address the problem without having to test the test.
There was a problem hiding this comment.
OK. That makes sense although it would make me more comfortable if something did an assertion - like that the len of the list that's returned from this function is the right number or something. But that seemed a lot harder to do.
|
👍 on tests modulo a comment, thanks @singingwolfboy :) |
|
Can you clean up the pep8/pylint stuff? |
|
Still a few Pylint things. Otherwise, 👍 |
…files Skip tilde files when importing
* Add ORA2 video upload option openedx#2375 (openedx#2417) * Modified courseware page look and feel openedx#2377 (openedx#2409) * Modified about page openedx#2379 (openedx#2413) * Fix biz bugs. openedx#2404 (openedx#2406) * Fix display width of popup. (openedx#2384) (cherry picked from commit a43c935a575d15fcf629f0edeb81446c778cda95) * Validate duplicate url-code. * Fix bug, when course not found. * Fix order of course as CourseOverview. * Remove additional-info count from contract grid. openedx#2419 (openedx#2437) * Fix order global course. openedx#2420 (openedx#2421) * Add additional info register. openedx#2419 (openedx#2433) * fix survey csv character encode problem openedx#2380 (openedx#2434) * Fix bokchoy for LoginCodeEnabledBizSurveyTest. (openedx#2457) * Fix register students confirm message. (openedx#2461) * Add command to check playback_log. openedx#2438 (openedx#2445) * Fix courseware page lookandfeel (openedx#2446, openedx#2439, openedx#2452, openedx#2453) * Fix box-shadow of sequence-nav-button. openedx#2453 (openedx#2467) * Fix bugs. openedx#2462 openedx#2463 (openedx#2464) * Fix password message in register students page. * Fix glass pane of processing when register additional item. * Fix display width of popup. (openedx#2466) * Fix min-width of sequence-nav. openedx#2468 (openedx#2469) * Fix isRegistered javascript in about page openedx#2470 (openedx#2471)
A longstanding bug has been that *~ files are imported, and can overload intended content. This affects, for example, the info and about page XML files, and causes problems for anyone importing .tar.gz files into Studio, or using the LMS+mongo workflow, or the XML files workflow.
This PR corrects the bug by skipping *~ files on import.