Qcodes icon indicating copy to clipboard operation
Qcodes copied to clipboard

Remove lower-case keysight folder

Open swernli opened this issue 2 years ago • 8 comments

Closes #1709

This fixes the case sensitivity issue that duplicate the Keysight folder with a lower case letter. While the net changes appear to just be the removal of the folder, there are five commits that effectively perform the removal through a sequence of renames that should work for both case sensitive file systems (Linux) and case insensitive systems (Windows). Note that newly cloned repos on Windows will no longer have the warning about a filename collision, but existing repos that update after this change will show an unstaged removal of qcodes/instrument_drivers/Keysight/init.py. Just reverting the change in VSCode or running git restore qcodes/instrument_drivers/Keysight/__init__.py from the cmdline will be enough to put the repo back into a clean state. Existing Linux and MacOS repos will be unaffected.

swernli avatar Jun 09 '22 13:06 swernli

@spauka Hope this works to resolve the issue we discussed!

swernli avatar Jun 09 '22 13:06 swernli

but existing repos that update after this change will show an unstaged removal of qcodes/instrument_drivers/Keysight/init.py

We reached the conclusion when this was first discovered that this is unacceptable which is why this has not been fixed see for example https://github.com/QCoDeS/Qcodes/pull/1710

jenshnielsen avatar Jun 09 '22 13:06 jenshnielsen

While that is fairly trivial to undo for experienced git users its very confusing for someone not familiar with git

jenshnielsen avatar Jun 09 '22 13:06 jenshnielsen

While that is fairly trivial to undo for experienced git users its very confusing for someone not familiar with git

Ah, I see. Taking that into account, I can see how this approach would be undesirable.

swernli avatar Jun 09 '22 13:06 swernli

That being said QCoDeS now has much more stable releases than last time we considered this so there will probably be fewer gic clones around so perhaps we should just do this

jenshnielsen avatar Jun 09 '22 13:06 jenshnielsen

Codecov Report

Merging #4248 (0810848) into master (93ef162) will increase coverage by 1.56%. The diff coverage is n/a.

:exclamation: Current head 0810848 differs from pull request most recent head 029aabe. Consider uploading reports for the commit 029aabe to get more accurate results

@@            Coverage Diff             @@
##           master    #4248      +/-   ##
==========================================
+ Coverage   68.22%   69.78%   +1.56%     
==========================================
  Files         333      296      -37     
  Lines       31671    42400   +10729     
==========================================
+ Hits        21607    29589    +7982     
- Misses      10064    12811    +2747     

codecov[bot] avatar Jun 09 '22 14:06 codecov[bot]

@jenshnielsen Turns out that if I include content in the Keysight/__init__.py file it will show up as a changed file and prevent the extraneous unstaged deletion on Windows. But if anyone ever removes the comment in there and changes it back into a blank file, then the unstaged deletion will come back. Probably too fragile to be useful in terms of an option, but I figured I'd reopen the PR just to show the possibility.

swernli avatar Jun 09 '22 14:06 swernli

@swernli Thanks for experimenting. I will have a chat with the other maintainers and try to decide if we should move forward with this. It would be really nice to get id of this extra folder

jenshnielsen avatar Jun 09 '22 14:06 jenshnielsen

@swernli I was intending to get back to this after the next release so took the liberty of reopening

jenshnielsen avatar Aug 22 '22 20:08 jenshnielsen

Landing this now since I discovered that any atttempt of editing __init__.py in either Keysight folder on windows will trigger this so this is wors that I thought

jenshnielsen avatar Sep 12 '22 09:09 jenshnielsen