PR: House cleaning #7785

CAM-Gerlach · 2018-08-29T22:57:03Z

Pull Request Checklist

Read and followed this repo's Contributing Guidelines
Based your PR on the latest version of the correct branch (master or 3.x)
Followed PEP8 for code style
Ensured your pull request hasn't eliminated unrelated blank lines/spaces,
modified the spyder/defaults directory, or added new icons/assets
Described your changes and the motivation for them below

Description of Changes

Waiting on @ccordoba12 to attach the source files for the Oxygen icons; then I'll make a few final changes that need to go last, and this will at long last be done.

Final changes:

~~Add source files for Oxygen icon them-derived icons from @ccordoba12 : BLOCKING~~
Harmonize LICENSE, etc. to LF
Move LICENSE/AUTHORS to .txt and NOTICES to NOTICE and update MANIFEST
Add appropriate boilerplate/list to LICENSE, AUTHORS, and init
Fix license title and other details in LICENSE and init.py
Clarify/add links to AUTHORS like for docs
Add template and instructions to NOTICES.txt and checkbox to PR template

Also, is there anything that needs to be done to make sure the NOTICE.txt always gets shipped with the package on both conda and pip?

Clean up a lot of old mess that could potentially cause problems, particularly legal ones, and make everything much cleaner, clearer, more transparent and well-documented.

I tried to minimize modifications to files that were moved in split-plugins to only those that were strictly necessary. The files added or changed in locations there moved were:

Add:

spyder/utils/help/js/mathjax/LICENSE.txt
spyder/utils/help/LICENSE.txt

Modify:

spyder/utils/sourcecode/kill_ring.py
spyder_profiler/widgets/profilergui.py
spyder/utils/help/__init__.py
spyder/utils/help/js/collapse_sections.py
spyder/utils/help/js/copy_button.py
spyder/widgets/variableexplorer/dataframeeditor.py

Once merged to master, I will follow this up with a PR making a few master-specific changes (mostly to file paths listed in NOTICE.txt), if they are not modified in the merge itself.

For the icon files that were removed, I took numerous steps to ensure to the greatest extent practical that they were not used anywhere and their removal would not cause other problems. Included among these steps for each of the removed icons (where "name" is the name of the icon, without the extension):

Searched for all instances of "name" and "name + extension" in 3.x and master w/ both Github and Spyder Find in Files
Used Git Blame to trace and verify past usage(s) of given icons
Inserted temporary code to both print a debug message and raise an exception if an icon was not found
Started Spyder with all panes enabled and tested basic functionality in every pane as well as the File Switcher and File Open/Save dialogs
Tested both Python 3 and Python 2 icon sets, and compared functionality with clean Spyder 3.2.8
Ran all tests (including slow) locally
Filetypes-specific removal justification:
- Added prior to July 2009, although a few additional types added as soon as ~2012
- Checked every relevant icon(), get_icon(), get_std_icon(), get_image_path() and filetypes call
- Tested file explorer with and without change in both icon sets with files of various extensions
- Checked every "*.png" string

Thanks!

ccordoba12 · 2018-09-07T04:49:45Z

README.md

+## Contributing and Credits
+
+Spyder was originally created by [Pierre Raybaut](
+https://github.com/PierreRaybaut), and is currently developed maintained by


Please remove developed here.

Will do, thanks for the catch.

ccordoba12 · 2018-09-07T04:50:59Z

img_src/Oxygen_Icon_Set/LICENSE.txt

@@ -0,0 +1,25 @@
+Oxygen Icon Theme SVG Sources


Please uncapitalize the name of this directory, i.e. leave it as oxygen_icon_set.

Okay, sure.

ccordoba12 · 2018-09-07T04:54:36Z

spyder/utils/help/LICENSE.txt

+Help Utilities License Information
+##################################
+
+Source files in this directory originate from sources other than Spyder itself,


This only applies to our MathJax copy, or am I mistaken?

Sorry, some files were also taken from somewhere else.

This warning was inspired by that originally present in the init,py in this directory.

Of the files contained within this directory, conf.py, sphinxify.py and templates/layout.html are from Sphixify, part of the Sage project; js/collapse_sections.js is from the Cloud Sphinx theme, js/copy_button.js is taken from Python's doc site, js/jquery.js is the entire JQuery and SizzleJS libraries, tutorial.rst and the contents of the static/images/ directory are originally copyright Hans Fanghor and dual-licensed, and there's of course MathJax as you mentioned; furthermore, every one of those is at least in part under a license other than MIT. The only Spyder-original files are the relatively trivial html/usage.html and html/warning.html, the remaining few files in the js/ directory, and those in the static/css/ directory.

However, I can clarify it a bit, along the lines of "Many source files in this directory..."

Done as above.

ccordoba12 · 2018-09-07T04:59:34Z

Also, don't forget to merge with master to fix our tests in Circle.

CAM-Gerlach · 2018-09-07T06:46:04Z

Also, don't forget to merge with master to fix our tests in Circle.

@ccordoba12 Did something further change in the last 24 hours that would break any further tests? I merged it with 3.x a day ago to fix the failing tests, which indeed did so as of then.

jnsebgosselin · 2018-09-07T15:45:14Z

spyder/app/mainwindow.py

-        msgBox.exec_()
+        msgBox.setTextInteractionFlags(
+            Qt.LinksAccessibleByMouse | Qt.TextSelectableByMouse)
+        msgBox.setModal(False)


I know we all agreed on making the About Spyder dialog not modal, but I think this is kind of a ninja change doing it so here. I think this should be part of a separate PR with its own associated Issue where this can be documented and discussed.

Several dialogs are modal in Spyder (Dependencies, Check for updates, Report issue, Python path manager, etc...). It is a little bit inconsistent to make the About dialog the only one not modal and so, I think this needs to be discussed/documented in an Issue first.

Thanks for bringing this up, and sorry about that. Actually, I only did so in the first place since it was extremely annoying having it be modal when I was trying to go back and forth testing the changes. Then, I found a bug in the dialog (as noted in the commit message), that the links no longer worked after your change in your recent PR making the text selectable, so I had to fix that so the link to the legal notices and author attributions would actually work, as is of course required for them to fulfill their legal purpose. After I'd already had to make changes I apparently left the modal flag there as well; at this point its been so long and so much I can't 100% remember if it was a conscious decision or not.

The dependencies dialog should definitely not be modal as well, since it serves the same function as the about dialog (displaying useful information). However, the others are more arguable. In any case, I can open a separate issue to discuss it; for now I'll just revert this change.

Then, I found a bug in the dialog (as noted in the commit message), that the links no longer worked after your change in your recent PR making the text selectable

Thanks for catching that!

The dependencies dialog should definitely not be modal as well, since it serves the same function as the about dialog (displaying useful information). However, the others are more arguable. In any case, I can open a separate issue to discuss it; for now I'll just revert this change.

I think the About, Dependencies and Report issue are useful to use in a same workflow and should not be modal.

I'll let you open an issue where this can be discussed in more details.

jnsebgosselin · 2018-09-07T17:52:06Z

I'm in the process of refactoring the About Spyder dialog window to implement #7844 and the content of this Pull request gave me an idea.

Would it be a good idea to add more information in the About Spyder dialog with a tab widget.

Something like in Inkscape for example:

CAM-Gerlach · 2018-09-07T20:04:27Z

@jnsebgosselin Fantastic idea!!! I had been eventually planning something like that, but as you mentioned earlier it would be well beyond the scope of this already bloated PR.

I was thinking it would be good to have a tab to display the license, the authors file (would we want to include every Github contributor though, or just display it as it is now and have a clickable link to the Github contributors page), and the notices file as scrollable/selectable plain text. It would be nice if the bare links could be made clickable, but that's not a priority. Of course, this makes it even more important for the dialog to be non-modal.

We could then remove the last paragraph of the current about dialog, since the authors would cover it. The first tab would be the about dialog more or less as it is now, and we could add the splash screen graphic in there at the top or bottom if we needed something to fill the space; and/or we'd have the room to list Spyder, OS, Qt, etc. versions in the same format as on Github, as a bulleted list, so users could simply copy and paste it directly if we wanted. We could also consider incorporating the dependencies dialog as a tab too, but that might be pushing it too far.

CAM-Gerlach · 2018-09-07T20:16:16Z

@ccordoba12 With regard to the failing test only on the pip build of Py3.6 Linux on Travis, the apparent cause is the new version of nbconvert, 5.4.0, which adds a shebang line at the top of the output Python file that results in the test_open_notebooks_from_project_explorer failing, as it checks for the file exactly matching the expected text which doesn't include the shebang. Should I create a new issue/PR to fix this in a straightforward manner (checking for the text including the shebang if nbconvert.__version__ >= 5.4.0), or is there a more robust way to handle this that won't break in the future if something else changes?

jnsebgosselin · 2018-09-08T13:52:51Z

@jnsebgosselin Fantastic idea!!! I had been eventually planning something like that, but as you mentioned earlier it would be well beyond the scope of this already bloated PR.

Excellent, I'll open an issue so we can get discussed this and I'll implement it with your help in a PR if it is approved.

ccordoba12 · 2018-09-08T21:24:27Z

NOTICE.txt

+===================
+
+
+


Remove two blanks here.

@ccordoba12 As I implied above in response to your previous comment to this effect, I've already revised the introduction to eliminate or minimize blank lines where possible. Therefore, some of these initial comments are already out of date.

Ok, then please merge with 3.x to fix the failure in Travis and push your new changes. Thanks!

Well, I need to address your other comments first, which is why I didn't just push to begin with. Thanks for fixing the Travis failure, BTW.

Done; pushed.

ccordoba12 · 2018-09-08T21:24:36Z

NOTICE.txt

+
+Overview
+--------
+


Remove one blank.

ccordoba12 · 2018-09-08T21:25:55Z

NOTICE.txt

+New Entry Template
+------------------
+
+


Remove one blank.

Well, you get the idea. There a are lot of unneeded blanks in this file. Please remove them to make it more tight.

I slimmed down the lines at the top (as in your first few comments), as well as trimming 1-2 blanks (down to the minimal 2 required to maintain consistency and clarity between major elements of each entry) throughout the whole document between the top- and section-level headings and the text below, and likewise did the same between all the text and the section separator lines. However, as I elaborated in my rationale for it being plain text and not Markdown/rst above, with regard to the the remaining

unneeded blanks

that I haven't removed are needed and have meaning and purpose. For this file more than any other, meaning, purpose, and unambiguous clarity take absolute precedence over aesthetics, unless the aesthetics both serve and do not conflict with those ends.

To that end, the line breaks serve a the primary purpose of clearly establishing the textual hierarchy and unambiguously defining the format of each entry and section: Four lines between sections; three lines between entries; two lines between top-level elements of each entry. Particularly, the latter (on which the higher levels were based) was necessary due to the vital need to separate the descriptive text copied from the source and that added, which often contains blank lines as part of its verbatim native formatting.

In light of your request, I carefully considered whether the two line breaks between the headings and the content, and between the content and the ----- separator, could be removed, but ultimately concluded that it would be both inconsistent with the above separation and hierarchy convention; looks odd, as every other element is separated by two lines; and could also lead to some ambiguity particularly in the license full text section.

Furthermore, they are intended to aid in avoiding merge conflicts for two different PRs adding new entries by ensuring at least one blank line is present in the original for any addition of a new entry/section (one line on each side, and one between on which the change is nominally made) to avoid triggering git's conflict detection (which relies on at least one blank line between any parallel changes); I haven't extensively tested how much this helps in practice, but it certainly won't hurt. Finally, there is the practical aspect, that any further changes (which would necessarily be to the above convention, and thus have cascading effects) would involve changing many hundreds of more lines of the source, and would require touching every single entry (greatly complicating any future change that involved reverting or otherwise altering most of the 40+ commits in this PR).

ccordoba12 · 2018-09-08T23:57:38Z

NOTICE.txt

+=============================
+
+
+Font Awesome 4.7 (via QtAwesome)


This makes no sense. We don't distribute FontAwesome. Why do we need this here?

This notice needs to be moved to QtAwesome. Please remove it from here.

This makes no sense. We don't distribute FontAwesome. Why do we need this here?

A very good question, and one on which I initially came to the same conclusion as you—I didn't want to "open that can of worms" for the same reasons you probably don't. However, on closer inspection there were very particular reasons why FA is the one exception to not listing anything that isn't physically included in the Spyder repo, reasons which make much more sense if one considers that "distribution" is not always synonymous with what actually matters under copyright law—that is, whether the original work is a meaningful part of the end product and its presentation.

Even if not physically included in the package, they comprise the vast majority of the standard Spyder iconset that is actively displayed as part of Spyder's GUI itself (rather than as output, data, or reprisentations thereof) to the end user, forming an integral piece in the user-visible end-product that is Spyder, as much if not more so than any other icon that is included. If they weren't both graphical, an icon font (rather than a typographical one) and made a part of Spyder's core graphical UI (rather than e.g. as part of user output), then we could get away with it, but as it stands they deserve the same treatment as any other Spyder icon, as they are as much a part as anything of the visual pastiche that is Spyder.

Is it still somewhat gray? Yes, it's not 100% certain the courts would rule in their favor, but given there's some very clear distinctions with this specific case that prevent it from becoming a slippery slope and applying to any (other) Spyder dependencies (and recall, it is specific to Font Awesome the asset, not QtAwesome the dependency—I don't list the details for that) it costs us little to nothing to add it, shouldn't require any further maintenance, and is unlikely at best to apply to anything in the future (and even if arguably so, doesn't obligate us to do so).

This notice needs to be moved to QtAwesome

Indeed it does; it certainly needs to go there as well.

It also has the benefit of clearly documenting, in the same consistent place, exactly where all of Spyder's primary iconset is derived from, for both users and developers interested in finding out more (or even using QtAwesome). Otherwise, one has to dig through the code to find out exactly where they're originally from, and even then it may not be 100%. So again, even if this were somehow "out of date", still better to have something than nothing as a useful starting point for those curious.

Or in the words of the Zen of Python, which couldn't be more true when it comes to legal and attribution matters, and sums up in one line most of my decisions here:

Explicit is always better than implicit.

ccordoba12

Thanks for helping us with this @CAM-Gerlach!

CAM-Gerlach · 2018-09-09T17:16:59Z

@ccordoba12 Thanks for putting up with my stubbornness; you have the patience of a saint.

Was the merge that bad?

ccordoba12 · 2018-09-09T17:18:32Z

Nop, fortunately it was quite easy 👍

CAM-Gerlach · 2018-09-09T17:20:55Z

Nop, fortunately it was quite easy 👍

Well then...Mission AcCAMplished!

xD

jnsebgosselin · 2018-09-10T02:17:17Z

Good job 👍

CAM-Gerlach added 30 commits August 24, 2018 23:56

Remove unnessesary vitables and pythonxy icons

b6aa16e

Remove no longer used PyDev and SciPy icons

55bbcdf

Removed no-longer-needed Qt external tools icons

60a83c8

Remove no longer used filetype icons after considerable testing

c386485

Replace loading sprites with unclear license with homemade ones

076eb49

Remove alternate Python logo

ad9b6ca

Remove use of Github logo

440365f

Move and expand addtional license info from LICENSE to NOTICES.txt

41a65b6

Add LICENSE.txt file in root of images directory

fb08938

Remove outline and py PNGs from img_src that are no longer used

490f144

Minor reorganization and additional clarification in NOTICES.txt

1edcdd9

Add Font Awesome information to NOTICES.txt and SIL OFL 1.1 text

dd5931d

Add SVG sources for Oxygen assets and accompyning text

f331332

Add MathJax license and information to NOTICES.txt and subdirectory

c41f349

Add jQuery and Sizzle, along with GPLv2 and BSD-3 to NOTICES.txt

518e98e

Fix headers/docstrings for QCrash and Githubpy and add to NOTICES

847ed23

Move QCrash Github tests into more appropriate directory

868be7b

Add Sphinxify and related information to NOTICES.txt

5fee34f

Add IPython and Jupyter QtConsole to headers and NOTICES.txt

ef505a4

Add BinaryOrNot headers and to NOTICES.txt, and version numbers

b82016f

Add LICENSE.txt and README.rst to externals dir and info to __init__

2a6bdcb

Add Twisted copyright to headers and NOTICE.txt

fcbf413

Add LICENSE.txt and __init__ headers in Help utils directory

d79df69

Add collapse sections from Sphinx Cloud theme headers and NOTICE.txt

6005290

Add headers and NOTICE.txt info for copybutton from PSF docs theme

565e4fa

Tweak headers for WaitingSpinner and add to NOTICE.txt

394d60c

Update dataframeeditor header and NOTICE.txt information

c9a665c

Update missing/placeholder copyright statements in gettext files

2fe7ed7

Update About dialog w/ copyright info etc and fix links not working

47b95a7

Add specific legal details to README where required or requested

0a42a04

ccordoba12 reviewed Sep 7, 2018

View reviewed changes

CAM-Gerlach added 3 commits September 7, 2018 01:58

Rename Oxygen Icon set directory in img_src to lower case

7c14181

Minor text tweaks in README and LICENSE for clarity

5ce0d24

Revert LF harmonization and re-organization in MANIFEST and fix bug

8c39472

jnsebgosselin reviewed Sep 7, 2018

View reviewed changes

Revert About dialog change to non-modal

d80242a

CAM-Gerlach mentioned this pull request Sep 7, 2018

Make dialogs non-modal where possible #7857

Open

ccordoba12 reviewed Sep 8, 2018

View reviewed changes

NOTICE.txt Outdated

Overview

--------

Copy link

Member

ccordoba12 Sep 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove one blank.

ccordoba12 reviewed Sep 8, 2018

View reviewed changes

CAM-Gerlach added 2 commits September 8, 2018 18:05

Remove some non-vital blank lines in the NOTICE.txt file

f0bba41

Merge with 3.x to fix remaining test failure

3f6cfc5

ccordoba12 reviewed Sep 8, 2018

View reviewed changes

ccordoba12 approved these changes Sep 9, 2018

View reviewed changes

ccordoba12 merged commit e904592 into spyder-ide:3.x Sep 9, 2018

ccordoba12 added a commit that referenced this pull request Sep 9, 2018

Merge from 3.x: PR #7785

85bef64

CAM-Gerlach deleted the house-cleaning branch September 12, 2018 02:27

CAM-Gerlach mentioned this pull request Nov 14, 2019

License issue with requests and requests3 psf/requests#5262

Closed

		=============================


		Font Awesome 4.7 (via QtAwesome)

		===================

		New Entry Template
		------------------

PR: House cleaning #7785

PR: House cleaning #7785

Conversation

CAM-Gerlach commented Aug 29, 2018 • edited Loading

Pull Request Checklist

Description of Changes

Choose a reason for hiding this comment

CAM-Gerlach Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Sep 7, 2018

CAM-Gerlach commented Sep 7, 2018 • edited Loading

jnsebgosselin Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsebgosselin commented Sep 7, 2018

CAM-Gerlach commented Sep 7, 2018

CAM-Gerlach commented Sep 7, 2018

jnsebgosselin commented Sep 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

CAM-Gerlach Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

ccordoba12 left a comment

Choose a reason for hiding this comment

CAM-Gerlach commented Sep 9, 2018

ccordoba12 commented Sep 9, 2018

CAM-Gerlach commented Sep 9, 2018

jnsebgosselin commented Sep 10, 2018

CAM-Gerlach commented Aug 29, 2018 •

edited

Loading

CAM-Gerlach Sep 7, 2018 •

edited

Loading

CAM-Gerlach Sep 7, 2018 •

edited

Loading

CAM-Gerlach Sep 7, 2018 •

edited

Loading

CAM-Gerlach commented Sep 7, 2018 •

edited

Loading

jnsebgosselin Sep 7, 2018 •

edited

Loading

CAM-Gerlach Sep 8, 2018 •

edited

Loading

CAM-Gerlach Sep 8, 2018 •

edited

Loading

CAM-Gerlach Sep 9, 2018 •

edited

Loading

CAM-Gerlach Sep 9, 2018 •

edited

Loading