'Re: Review Request 113790: added jshint and restructured jsutils'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kwrite-devel
Subject:    Re: Review Request 113790: added jshint and restructured jsutils
From:       "Phil Schaf" <flying-sheep () web ! de>
Date:       2013-11-12 16:28:40
Message-ID: 20131112162840.590.90590 () vidsolbach ! de
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

> On Nov. 12, 2013, 1:30 p.m., Alex Turbov wrote:
> > addons/kate/pate/src/plugins/js_utils/js_settings.py, line 25
> > <http://git.reviewboard.kde.org/r/113790/diff/2/?file=212559#file212559line25>
> > 
> > Why do you python guys always do that? I mean move action declaration into a \
> > separate (and useless) class/dict? I can understand it from developers viewpoint: \
> > to keep all declaration details at single location, so when you want to edit smth \
> > you know exactly where to go and fix... But, from reader side it is really \
> > inconvinient: when I see @kate.action dcorator, I can't get details just looking \
> > at it, I have to find where that KATE_ACTIONS defined, and find particulat \
> > entry...
> 
> Phil Schaf wrote:
> so should i inline it? i'm ok with that.
> 
> actually i even think there should be a better way than how the kate.action \
> decorator is used right now: right now, if used in a submodule, one has to import \
> the submodule just to execute the decorator's side effect. elsewhere it's a python \
> philosophy not to have import side effects. 
> Alex Turbov wrote:
> Personally I see only problems w/ this approach, and no benefits (except "single \
> place for all action declarations"). It is why I asked.... maybe there is smth else \
> I don't see %) Reading your answer, I realize that there is nothing else :)  
> What do you mean "in submodule"? A module which is used from a pate plugin? and \
> containing an action declaration? 
> Phil Schaf wrote:
> in the js_utils plugin directory, there's a js_utils.py which gets executed \
> automatically. from it, you can reference other modules in the same directory by \
> name ("from js_lint import ..."). those others are what i called submodules. 
> up to now, the actions were logically grouped into the submodules they belong to \
> (js_lint.py ? @kate.action def js_lint). but sure, centralizing actions works, too. \
>  Alex Turbov wrote:
> Now, when we have a .desktop files, it would be easy to add smth like \
> X-Python-Aux-Modules and import all listed modules in addition to the main one \
> (listed in X-KDE-Library)... If it helps to resolve your concern, I can add it \
> really quick... 
> Phil Schaf wrote:
> oh, i like the structure more like it is now: create all actions and config pages \
> in the main module, and initialize everything there, too. 
> one can modularize by putting the main work into submodules and just make the \
> action functions tiny wrappers around them. you'll see in the new diff i'll upload \
> momentarily. 
> what i'd rather like to se is if we could make plugins proper python modules, so \
> that loading a plugin does the same as "import pluginname". that would imply that \
> single-file plugins stay like they are, and directory-based plugins change by \
> requiring absolute imports (i.e. explicitly relative imports like "from .submodule \
> import symbol"), and that the main .py file is no longer pluginname/pluginname.py, \
> but pluginname/__init__.py 
> Alex Turbov wrote:
> now X-KDE-Library supposed to be a path to the .py file of the plugin. I can fix \
> the engine to handle dirs as well... and if listed path is actually a dir, load \
> __init__.py instead... -- will it looks like "python way to load modules"??

currently, the directory is added to the python path, else, e.g. "import js_lint" \
wouldn't work from "js_utils/js_utils.py".

and because of that, it's not the same. i propose that just the directories where \
plugins reside in are added to the python path, and then we call py.moduleImport

so change the check in tryLoadEnabledPlugins to be "is directly in a plugin \
directory, and either a .py file or a directory containing __init__.py"

then change loadModules to be basically "py.moduleImport(plugin.pythonModuleName())" \
for each active module. (without descending into its directory and without adding the \
module path to the python path)

---

so if we have in our plugin directory "abc/plugins/"

foo.py
bar/__init__.py
bar/baz.py

we just have to add "abc/plugins" to the python path, then call from C++: \
"py.moduleImport("foo"); py.moduleImport("bar")"

and "import baz" should work *nowhere*, while "from .baz import symbol" should work \
inside every script called "bar/*.py"

- Phil

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113790/#review43515
-----------------------------------------------------------

On Nov. 12, 2013, 3:54 p.m., Phil Schaf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113790/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2013, 3:54 p.m.)
> 
> 
> Review request for Kate.
> 
> 
> Repository: kate
> 
> 
> Description
> -------
> 
> restructured JSUtils and added an option to use JSHint instead of the opinionated \
> JSLint. 
> removed dependency (we don't need a clumsy JS wrapper using system calls if Qt \
> already has a JS runtime) 
> removed necessity ro restart kate after changing settings.
> 
> i created many useful utilities in the process which simplify settings and JS. \
> maybe we could add them to libkatepate? 
> 
> Diffs
> -----
> 
> addons/kate/pate/src/plugins/js_utils/js_config_page.py PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/js_engine.py PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/js_lint.py PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/js_settings.py 4272372 
> addons/kate/pate/src/plugins/js_utils/js_snippets.py d684b25 
> addons/kate/pate/src/plugins/js_utils/js_utils.desktop PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/js_utils.py ee879a0 
> addons/kate/pate/src/plugins/js_utils/js_utils.ui 6fd8b20 
> addons/kate/pate/src/plugins/js_utils/jshint.js PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/jslint.py 03fea05 
> addons/kate/pate/src/plugins/js_utils/json_pretty.py 135e72d 
> addons/kate/pate/src/plugins/CMakeLists.txt bddc725 
> addons/kate/pate/src/plugins/js_utils/fulljslint.js PRE-CREATION 
> addons/kate/pate/src/plugins/js_utils/js_autocomplete.py 40b1523 
> 
> Diff: http://git.reviewboard.kde.org/r/113790/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Phil Schaf
> 
> 

[Attachment #5 (text/html)]

<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;">  <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="http://git.reviewboard.kde.org/r/113790/">http://git.reviewboard.kde.org/r/113790/</a>
  </td>
    </tr>
   </table>
   <br />

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On November 12th, 2013, 1:30 p.m. UTC, <b>Alex \
Turbov</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">  

<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="http://git.reviewboard.kde.org/r/113790/diff/2/?file=212559#file212559line25" \
style="color: black; font-weight: bold; text-decoration: \
underline;">addons/kate/pate/src/plugins/js_utils/js_settings.py</a>  <span \
style="font-weight: normal;">

     (Diff revision 2)

    </span>
   </th>
  </tr>
 </thead>

 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">23</font></th>  <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="n">KATE_ACTIONS</span> <span class="o">=</span> <span class="p">{</span><span \
class="s">&#39;insertReady&#39;</span><span class="p">:</span> <span \
class="p">{</span><span class="s">&#39;text&#39;</span><span class="p">:</span> <span \
class="s">&#39;jQuery Ready&#39;</span><span class="p">,</span></pre></td>  <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">25</font></th>  <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="k">class</span> <span class="nc">KATE_ACTIONS</span><span \
class="p">:</span></pre></td>  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why do you python guys \
always do that? I mean move action declaration into a separate (and useless) \
class/dict? I can understand it from developers viewpoint: to keep all declaration \
details at single location, so when you want to edit smth you know exactly where to \
go and fix... But, from reader side it is really inconvinient: when I see \
@kate.action dcorator, I can&#39;t get details just looking at it, I have to find \
where that KATE_ACTIONS defined, and find particulat entry... </pre>  </blockquote>

 <p>On November 12th, 2013, 1:52 p.m. UTC, <b>Phil Schaf</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">so should i inline it? \
i'm ok with that.

actually i even think there should be a better way than how the kate.action decorator \
is used right now: right now, if used in a submodule, one has to import the submodule \
just to execute the decorator's side effect. elsewhere it's a python philosophy not \
to have import side effects.</pre>  </blockquote>

 <p>On November 12th, 2013, 2:03 p.m. UTC, <b>Alex Turbov</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Personally I see only \
problems w/ this approach, and no benefits (except &quot;single place for all action \
declarations&quot;). It is why I asked.... maybe there is smth else I don&#39;t see \
%) Reading your answer, I realize that there is nothing else :) 

What do you mean &quot;in submodule&quot;? A module which is used from a pate plugin? \
and containing an action declaration?</pre>  </blockquote>

 <p>On November 12th, 2013, 2:12 p.m. UTC, <b>Phil Schaf</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">in the js_utils plugin \
directory, there's a js_utils.py which gets executed automatically. from it, you can \
reference other modules in the same directory by name ("from js_lint import ..."). \
those others are what i called submodules.

up to now, the actions were logically grouped into the submodules they belong to \
(js_lint.py ? @kate.action def js_lint). but sure, centralizing actions works, \
too.</pre>  </blockquote>

 <p>On November 12th, 2013, 2:55 p.m. UTC, <b>Alex Turbov</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Now, when we have a \
.desktop files, it would be easy to add smth like X-Python-Aux-Modules and import all \
listed modules in addition to the main one (listed in X-KDE-Library)... If it helps \
to resolve your concern, I can add it really quick...</pre>  </blockquote>

 <p>On November 12th, 2013, 3:37 p.m. UTC, <b>Phil Schaf</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">oh, i like the structure \
more like it is now: create all actions and config pages in the main module, and \
initialize everything there, too.

one can modularize by putting the main work into submodules and just make the action \
functions tiny wrappers around them. you'll see in the new diff i'll upload \
momentarily.

what i'd rather like to se is if we could make plugins proper python modules, so that \
loading a plugin does the same as "import pluginname". that would imply that \
single-file plugins stay like they are, and directory-based plugins change by \
requiring absolute imports (i.e. explicitly relative imports like "from .submodule \
import symbol"), and that the main .py file is no longer pluginname/pluginname.py, \
but pluginname/__init__.py</pre>  </blockquote>

 <p>On November 12th, 2013, 3:47 p.m. UTC, <b>Alex Turbov</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">now X-KDE-Library \
supposed to be a path to the .py file of the plugin. I can fix the engine to handle \
dirs as well... and if listed path is actually a dir, load __init__.py instead... -- \
will it looks like &quot;python way to load modules&quot;??</pre>  </blockquote>

</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">currently, \
the directory is added to the python path, else, e.g. "import js_lint" wouldn't work \
from "js_utils/js_utils.py".

and because of that, it's not the same. i propose that just the directories where \
plugins reside in are added to the python path, and then we call py.moduleImport

so change the check in tryLoadEnabledPlugins to be "is directly in a plugin \
directory, and either a .py file or a directory containing __init__.py"

then change loadModules to be basically "py.moduleImport(plugin.pythonModuleName())" \
for each active module. (without descending into its directory and without adding the \
module path to the python path)

---

so if we have in our plugin directory "abc/plugins/"

foo.py
bar/__init__.py
bar/baz.py

we just have to add "abc/plugins" to the python path, then call from C++: \
"py.moduleImport(&quot;foo&quot;); py.moduleImport(&quot;bar&quot;)"

and "import baz" should work *nowhere*, while "from .baz import symbol" should work \
inside every script called "bar/*.py"</pre> <br />

<p>- Phil</p>

<br />
<p>On November 12th, 2013, 3:54 p.m. UTC, Phil Schaf wrote:</p>

<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;">  <tr>
  <td>

<div>Review request for Kate.</div>
<div>By Phil Schaf.</div>

<p style="color: grey;"><i>Updated Nov. 12, 2013, 3:54 p.m.</i></p>

<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
kate
</div>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">restructured JSUtils and added an option to use JSHint instead of the \
opinionated JSLint.

removed dependency (we don't need a clumsy JS wrapper using system calls if Qt \
already has a JS runtime)

removed necessity ro restart kate after changing settings.

i created many useful utilities in the process which simplify settings and JS. maybe \
we could add them to libkatepate?</pre>  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>addons/kate/pate/src/plugins/js_utils/js_config_page.py <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_engine.py <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_lint.py <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_settings.py <span style="color: \
grey">(4272372)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_snippets.py <span style="color: \
grey">(d684b25)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_utils.desktop <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_utils.py <span style="color: \
grey">(ee879a0)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_utils.ui <span style="color: \
grey">(6fd8b20)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/jshint.js <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/jslint.py <span style="color: \
grey">(03fea05)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/json_pretty.py <span style="color: \
grey">(135e72d)</span></li>

 <li>addons/kate/pate/src/plugins/CMakeLists.txt <span style="color: \
grey">(bddc725)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/fulljslint.js <span style="color: \
grey">(PRE-CREATION)</span></li>

 <li>addons/kate/pate/src/plugins/js_utils/js_autocomplete.py <span style="color: \
grey">(40b1523)</span></li>

</ul>

<p><a href="http://git.reviewboard.kde.org/r/113790/diff/" style="margin-left: \
3em;">View Diff</a></p>

  </td>
 </tr>
</table>

  </div>
 </body>
</html>

_______________________________________________
KWrite-Devel mailing list
KWrite-Devel@kde.org
https://mail.kde.org/mailman/listinfo/kwrite-devel

[prev in list] [next in list] [prev in thread] [next in thread] 