PR: Make project pip installable and add @import support #2

danbradham · 2018-05-01T23:15:48Z

Fixes #3
Fixes #4
Fixes #8
Fixes #9

Hello, great project!

Found this while attempting to use libsass for a qt project and ran into a couple of issues.

First, I couldn't pip install qtsass. This fork has a standard setup.py that does not use cx_freeze. This makes it easier for me to install on windows/linux. I'm not sure if qtsass requires cx_freeze for spyder, but, for me it makes more sense to have a standard setup.py.

This fork also introduces an importer callback passed to libsass.compile that conforms scss imported using @import statements.

Finally, I introduced an abstraction around conforming css to qss and qss to css. This should make it easier to support more qss specific syntax in the future. For example, there could be a Conformer for qlineargradient that transforms the qss syntax:

qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1, stop: 0.1 blue, stop: 0.8 green)

to your libsass qlineargradient function syntax:

qlineargradient(0, 0, 0, 1, (0.1 blue, 0.8 green))

Removed NestedSelectorConformer - this was introduced based on a red herring - real issue was imported qss not being conformed - Conformer abstraction still valid

goanpeca · 2018-05-02T00:13:58Z

Hey @danbradham thanks for the help, we took ownership of this project since we want to start styling Spyder using QSS, but have not gotten around to it yet for lack of time. Let me get back to you by the end of this week. Ok ?

Sorry but I cannot make this sooner

CAM-Gerlach

A few comments mostly related to adherence to standards, conventions and style guides.

CAM-Gerlach · 2018-05-02T01:33:21Z

qtsass/qtsass.py

@@ -10,12 +13,107 @@
 logger = logging.getLogger(__name__)


+class Conformer(object):
+    '''Base class for all Text transformations'''


With regard to all your docstrings, thanks for including such detailed documentation. Per PEP 257 (the docstring standard), docstrings are required to use double quotes """ , not single, which is adhered to elsewhere in the codebase. Also per PEP 257, all docstrings should end with a period, and at least according to that (I'm not sure its strictly followed, either here or in the rest of the world) there should be a space both before and after a class docstring, for symmetry.

CAM-Gerlach · 2018-05-02T01:36:38Z

qtsass/qtsass.py

+
+
+def css_conform(input_str):
+    '''Conform qss to valid scss.


Somewhat a matter of taste, but the Spyder dev generally seems to agree that putting a linebreak before the summary line of a multi-line docstring is preferable as it is clearer and looks cleaner, and that seems to be the convention used elsewhere in the codebase. E.g.

""" Conform qss to valid scss. Blah blah blah """

CAM-Gerlach · 2018-05-02T01:37:41Z

qtsass/qtsass.py

-        if event.is_directory and os.path.samefile(event.src_path, self._watched_dir) or (
-           os.path.splitext(event.src_path)[1] == self._watched_extension):
+        # On Windows, only recompile if event's file has
+        #             the same extension as the input file


Rather strange indentation here...

CAM-Gerlach · 2018-05-02T01:40:47Z

qtsass/qtsass.py

+        '--watch',
+        action='store_true',
+        help=(
+            "Whether to watch source file "


IMO, I find it better to be explicit with boolean flags and the like, to avoid any ambiguity, especially given our highly international audience. I.e. "If set, will watch the source file and automatically recompile if it changes."

CAM-Gerlach · 2018-05-02T01:52:30Z

setup.py

+    name=qtsass.__title__,
+    version='0.1.0',
+    description=qtsass.__description__,
+    long_description=open('README.md', 'r').read(),


Worth passing encoding="utf-8" here? On Windows, it defaults to something stupid instead, so if non-ASCII chars are present now or in the future on the machine reading the file, mojibake will likely result.

- Use io.open with utf-8 encoding in setup.py - Use qtsass.__version__ in setup.py

CAM-Gerlach

Looking good; a few minor followup comments.

CAM-Gerlach · 2018-05-02T04:17:40Z

setup.py

    packages=find_packages(),
    entry_points={
-        'console_scripts': [
-            'qtsass = qtsass.qtsass:main'
+        "console_scripts": [


Just to be clear, the double-quotes requirement only refers to docstrings—Python has no general/accepted preference for single or double quotes for string literals. My personal convention is to use double in my own work, but most Spyder projects seem to prefer single as a rough convention except for specific circumstances. Not sure it matters hugely, but just wanted to make sure my earlier statement was clear.

Yeah you were clear previously, I was just falling in line with the rest of this project.

Since we took ownership of this project (we did not start this) that is why we have difference @CAM-Gerlach . But I think is good to homogenize :-)

Indeed, indeed. For clarity's sake, homogenize to " as used in this repo, or homogenize to ' as is de-facto "Spyder-style"?

CAM-Gerlach · 2018-05-02T04:20:12Z

qtsass/qtsass.py

-    Runs the to_css method of all Conformer subclasses on the input_str.
-    Conformers are run in order of definition.
+    """
+    Conform qss to valid scss. Runs the to_css method of all Conformer


The summary line should still be on its own line, separated by a blank like from the extended summary/description. See my comment on my first round of review for an example.

goanpeca · 2018-05-02T14:35:46Z

Hi @danbradham, just sent you an email...

goanpeca · 2018-05-02T16:11:06Z

setup.py

-from cx_Freeze import setup, Executable
+from setuptools import setup, find_packages
+from io import open
+import qtsass


To avoid having to import things, lets use
https://github.com/spyder-ide/loghub/blob/master/setup.py#L21

The idea is avoid having to import the package itself when running the installer

goanpeca · 2018-05-02T16:11:58Z

setup.py

+    name=qtsass.__title__,
+    version=qtsass.__version__,
+    description=qtsass.__description__,
+    long_description=open("README.md", "r", encoding="utf-8").read(),


We can clean this up a bit by moving out to a helper function.

https://github.com/spyder-ide/loghub/blob/master/setup.py#L34

goanpeca · 2018-05-02T16:12:31Z

setup.py

-    executables=[Executable("qtsass/qtsass.py")],
-    options={'build_exe': build_exe_options},
-    requires=['libsass', 'cx_Freeze', 'watchdog'],
+    name=qtsass.__title__,


This could be potentially problematic (having to import the package itself)

So I would prefer we just left the version on __init__.py and the rest here

goanpeca · 2018-05-02T16:13:52Z

setup.py

+    description=qtsass.__description__,
+    long_description=open("README.md", "r", encoding="utf-8").read(),
+    author=qtsass.__author__,
+    author_email="N/A",


Author :-) https://github.com/yann-lty

@yann-lty do you want as to include your email?

@danbradham lets add maintainer section and include your details

goanpeca

Added some additional comments :-)

goanpeca · 2018-05-02T16:14:11Z

setup.py

+        ]
+    },
+    classifiers=(
+        "Development Status :: 3 - Alpha",


maybe we can remove Alpha and go to official stable release?

goanpeca · 2018-05-02T16:15:41Z

test_qtsass.py

+from __future__ import absolute_import
+import unittest
+from textwrap import dedent
+from qtsass.qtsass import NotConformer, QLinearGradientConformer


For imports lets break them on sections like

https://github.com/spyder-ide/loghub/blob/master/loghub/core/formatter.py#L12

goanpeca · 2018-05-02T16:16:05Z

test_qtsass.py

+        self.assertEqual(c.to_css(self.qss_singleline_str), self.css_str)
+
+    def test_conform_multiline_str(self):
+        """QLinearGradientConformer multiline qss to css"""


Docstrings are sentences so they should be finished with a dot .

- Move most metadata into setup.py - Add VERSION_INFO to __init__.py - Use setup.py utility functions from loghub project

CAM-Gerlach · 2018-05-02T17:11:32Z

As I understand it, its a best practice to keep PRs narrow in scope and try to keep to one issue a PR, unless they are intertwined/related. Not that it matters much at this stage, but just wanted to put that out there.

CAM-Gerlach · 2018-05-02T17:13:48Z

LICENSE.txt

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) The Spyder Development Team


See my comment on #9 Also needs an AUTHORS.txt file.

danbradham · 2018-05-02T17:15:27Z

I agree with 1 issue per pull request, this one just got out of hand as it was kind of off the cuff. I'm going to stop after AUTHORS.txt

CAM-Gerlach

Some minor things

CAM-Gerlach · 2018-05-02T17:21:53Z

AUTHORS.txt

+
+The Spyder Project Contributors are composed of:
+
+* Pierre Raybaut <pierre.raybaut@gmail.com> (Original Spyder author).


Remove as requested

CAM-Gerlach · 2018-05-02T17:22:04Z

AUTHORS.txt

+The Spyder Project Contributors are composed of:
+
+* Pierre Raybaut <pierre.raybaut@gmail.com> (Original Spyder author).
+* Carlos Cordoba <ccordoba12@gmail.com> (Current maintainer).


Remove as requested

CAM-Gerlach · 2018-05-02T17:22:27Z

AUTHORS.txt

@@ -0,0 +1,9 @@
+* Yann Lanthony <N/A> (Original qtsass author).


You can put his Github link here if he doesn't want his email out there. Just tag him in the issue thread to get his permission and ask him.

CAM-Gerlach · 2018-05-02T17:24:23Z

Happens to all of us, heh...

CAM-Gerlach · 2018-05-02T17:47:04Z

AUTHORS.txt

@@ -0,0 +1,6 @@
+The Spyder Project Contributors are composed of:
+
+* Yann Lanthony <https://github.com/yann-lty> (Original qtsass author).


He should be listed separately above, just as you had it before, following the same convention as the boilerplate and LICENSE file.

goanpeca · 2018-05-02T20:03:43Z

qtsass/qtsass.py

 import time
+import sass


sass is a third party import, no?

goanpeca · 2018-05-02T20:05:18Z

Minor change then I can merge. As a rule, lets try to have someone different form the author of the PR, merging their own PRs

goanpeca · 2018-05-02T20:05:48Z

@danbradham also for next time pelase create a branch from master and work there, instead of directly working on your fork on master.

danbradham added 3 commits May 1, 2018 17:28

Introduce Conformer abstraction, support nested selectors

edb3a48

Support pip - no cx_freeze

914ed93

Added qss_importer - conforms imported qss files to valid scss

23c8953

Removed NestedSelectorConformer - this was introduced based on a red herring - real issue was imported qss not being conformed - Conformer abstraction still valid

CAM-Gerlach reviewed May 2, 2018

View reviewed changes

Tidy up docstrings

6be9fb8

- Use io.open with utf-8 encoding in setup.py - Use qtsass.__version__ in setup.py

CAM-Gerlach reviewed May 2, 2018

View reviewed changes

danbradham added 4 commits May 2, 2018 07:35

Tweak docs

1a494e5

Fixed missing comma in qlineargradient

f06e112

Add QLinearGradientConformer and conformer tests

0d3a46b

Fix qlineargradient with no stops

b3ac825

danbradham force-pushed the master branch from 437a7b4 to b3ac825 Compare May 2, 2018 14:18

Add test qlineargradient with vars

1103a23

ccordoba12 changed the title ~~pip installable - @import support~~ PR: Make project pip installable and add @import support May 2, 2018

CAM-Gerlach assigned danbradham May 2, 2018

goanpeca reviewed May 2, 2018

View reviewed changes

goanpeca added this to the v0.1.0 milestone May 2, 2018

danbradham added 4 commits May 2, 2018 13:01

Reorganize pkg metadata

4596700

- Move most metadata into setup.py - Add VERSION_INFO to __init__.py - Use setup.py utility functions from loghub project

close spyder-ide#8 add travis ci support

fa464e3

Use single quotes - spyder style

488f28f

closes spyder-ide#9 Add license and copyright boiler

61c346d

danbradham force-pushed the master branch from 838e0ed to 61c346d Compare May 2, 2018 17:10

CAM-Gerlach reviewed May 2, 2018

View reviewed changes

Add AUTHORS.txt

bc826d1

CAM-Gerlach reviewed May 2, 2018

View reviewed changes

Punctuation in test docs

78c6166

danbradham force-pushed the master branch from 18611fc to 78c6166 Compare May 2, 2018 17:26

danbradham added 4 commits May 2, 2018 13:32

Corrent authors

fdf4191

Added boiler to tests

9b976b3

Fix copyright in LICENSE.txt

56bb1f7

Temporarily use Yann's github url

1542123

CAM-Gerlach reviewed May 2, 2018

View reviewed changes

Yann at the top

b4e7ad1

danbradham force-pushed the master branch from 83ff612 to b4e7ad1 Compare May 2, 2018 17:56

goanpeca reviewed May 2, 2018

View reviewed changes

qtsass/qtsass.py Outdated

import time

import sass

Copy link

Member

goanpeca May 2, 2018

Choose a reason for hiding this comment

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

sass is a third party import, no?

Move import sass

c0281ef

goanpeca approved these changes May 2, 2018

View reviewed changes

goanpeca merged commit 327c49f into spyder-ide:master May 2, 2018

		@@ -0,0 +1,21 @@
		MIT License

		Copyright (c) The Spyder Development Team


		The Spyder Project Contributors are composed of:

		* Pierre Raybaut <pierre.raybaut@gmail.com> (Original Spyder author).

		@@ -0,0 +1,9 @@
		* Yann Lanthony <N/A> (Original qtsass author).

		@@ -0,0 +1,6 @@
		The Spyder Project Contributors are composed of:

		* Yann Lanthony <https://github.com/yann-lty> (Original qtsass author).

PR: Make project pip installable and add @import support #2

PR: Make project pip installable and add @import support #2

Conversation

danbradham commented May 1, 2018 • edited by goanpeca

goanpeca commented May 2, 2018

CAM-Gerlach left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach May 2, 2018 • edited

Choose a reason for hiding this comment

CAM-Gerlach left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented May 2, 2018

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

goanpeca left a 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 commented May 2, 2018

Choose a reason for hiding this comment

danbradham commented May 2, 2018

CAM-Gerlach left a 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 commented May 2, 2018

CAM-Gerlach May 2, 2018 • edited

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

goanpeca commented May 2, 2018

goanpeca commented May 2, 2018

danbradham commented May 1, 2018 •

edited by goanpeca

CAM-Gerlach May 2, 2018 •

edited

CAM-Gerlach May 2, 2018 •

edited