PR: Change the way %(date)s and %(username)s are dealt with in the template.py #6380

chengtian5huang · 2018-02-04T15:27:37Z

Take the part between tripe-double-quotes separately and deal with it separately we can replace %(date)s properly no matter what a man add in template.py

take the part between tripe-double-quotes seperately and deal with it seperately we can replace %(date)s properly no matter what a man add in template.py

pep8speaks · 2018-02-04T15:27:40Z

Hello @chengtian5huang! Thanks for updating the PR.

In the file spyder/plugins/editor.py, following are the PEP8 issues :

Line 1863:1: W293 blank line contains whitespace
Line 1869:13: E722 do not use bare except'
Line 1903:29: E231 missing whitespace after ','

In the file spyder/plugins/tests/test_template_renderer.py, following are the PEP8 issues :

Line 6:1: E302 expected 2 blank lines, found 0
Line 43:80: E501 line too long (87 > 79 characters)
Line 64:80: E501 line too long (87 > 79 characters)
Line 158:80: E501 line too long (87 > 79 characters)
Line 174:80: E501 line too long (86 > 79 characters)
Line 185:80: E501 line too long (87 > 79 characters)
Line 199:1: E305 expected 2 blank lines after class or function definition, found 0

Comment last updated on February 10, 2018 at 03:19 Hours UTC

ccordoba12 · 2018-02-04T22:30:15Z

Thanks for your contribution. There can be several triple quote sections in a template. How does your code deal with that?

chengtian5huang · 2018-02-05T11:51:05Z

improved my code now, it can handle all cases i hope.

to anwser ccordoba12's question:

if there are several triple quote sections in a template, it will check and check only(leave along other part of template) every section to replace vaild placeholder in the pre-set VARS.

and i have to change the name of these placeholders(eg change "date" to "date-spyder") in order to avoide over-replacing.

i describe in detail how it work under all cases as following:(skip this if u r not interested)

following "invaild"s means spyder DONT know how to replace it.to be specific:it is NOT in the VARS dict.
following "vaild"s means spyder know how to replace it.to be specific:it is in the VARS dict.

this problem can be divided into following cases

there is no invaild placeholder in the template.

replace them all, of course.

there are invaild placeholder in the template but all of them are in the outside of triple quote sections(no matter how many)

ignore these placeholders, they will remain the same

there are invaild placeholder in the template but all of them are in triple quote sections(no matter how many)

the triple quote sections which contain and contain only vaild placeholder will be dealt properly. all placeholders in them will be replaced.
the triple quote sections which contain invaild placeholder will be ignore. any of these placeholders wont be replaced.

there are invaild placeholder in the template but some of them are in triple quote sections(no matter how many), some of them are in the outside of triple quote sections(no matter how many).

only triple quote sections which contain and contain only vaild placeholders will be replaced.
the placeholder in the outside of triple quote sections wont be replaced.
all triple quote sections contain any invaild placeholders will be remain the same.

CAM-Gerlach

Aside from the minor notes to consider, any way you can add some simple tests for _replace_triple_quote, and maybe new(), to cover the scenarios you've laid out?

CAM-Gerlach · 2018-02-09T00:14:58Z

spyder/plugins/editor.py

@@ -1860,21 +1860,30 @@ def _clone_file_everywhere(self, finfo):
        for editorstack in self.editorstacks[1:]:
            editor = editorstack.clone_editor_from(finfo, set_current=False)
            self.register_widget_shortcuts(editor)
+


Delete extra blank spaces

CAM-Gerlach · 2018-02-09T00:15:41Z

spyder/plugins/editor.py

@@ -1860,21 +1860,30 @@ def _clone_file_everywhere(self, finfo):
        for editorstack in self.editorstacks[1:]:
            editor = editorstack.clone_editor_from(finfo, set_current=False)
            self.register_widget_shortcuts(editor)
+
+    def _replace_tripe_quote(self, m, v=None):


Maybe use slightly more descriptive parameter names than just single characters?

CAM-Gerlach · 2018-02-09T00:18:02Z

spyder/plugins/editor.py

+                from functools import partial
+                replace = partial(self._replace_tripe_quote, v=VARS)
+                pat = '"""(?:(?!""").|[\n\r])*"""'
+                text = re.sub(pat,


Perhaps a matter of taste, but any reason this can't all go in one line? It would only be 49 chars long...

chengtian5huang · 2018-02-09T04:14:59Z

@CAM-Gerlach
i put whole thing into new().
i dont know what do u mean by "simple tests". i cant use simple tests to deal with these.
only when

text = text % VARS

raise an exception i am able to know there is user-specified(which is invaild to spyder) placeholder.
the simpler way i can see is go down templete.py line by line.

    try:
        text = text % VARS
    except (KeyError,TypeError) as err:
        text = text.split('\n') # if Macintosh, use \r
        for i,line in enumerate(text):
            try:
                text[i] = line % VARS
            except:
                pass
        text = '\n'.join(text)

CAM-Gerlach

Sorry, but please revert your last commit, as it seems to have little to nothing to do with what I suggested while introducing two major regressions. It removes all whitespace in the entire file, (rather than just the one line you introduced), and nowhere did I say anything about moving your helper function to new(), its better design to keep it separate. Honestly, I'd just git reset --hard HEAD~ then git push -f and start again from there.

CAM-Gerlach · 2018-02-09T05:12:02Z

Sorry, but I think we've had a major misunderstanding here. Please see my review comment for more on that specifically.

i put whole thing into new().

Umm, what? Where exactly did I ever suggest you do this? It is better design to keep it separate as a helper function as you had it originally, so I have no idea what you thought it should be changed.

i dont know what do u mean by "simple tests". i cant use simple tests to deal with these.

I mean unit tests for your code, not anything to do with the program logic you've already written. As you seemed to have an understanding of various cases and how it should handle them, I figured it wouldn't be too difficult to write a few of them into proper tests. At the very least, testing your helper function should be easy to do. If you don't know what unit tests are, I suggest you at least make yourself familiar with them as they are a huge part of programming nowadays; pytest makes them very easy to write without a lot of extra baggage that other frameworks have.

only when text = text % VARS raise an exception i am able to know there is user-specified(which is invaild to spyder) placeholder. the simpler way i can see is go down templete.py line by line.

Not 100% sure what you're proposing nor why, but it seems clear it has nothing to do with what I requested, which were only minor changes and adding tests that had nothing to do with the core logic.

CAM-Gerlach

Thanks, although it would probably just make more sense to just git reset to before you made that first bad commit and force push that since the last two commits (that commit and the revert) are both consecutive and rather pointless, cluttering the commit history.

Changes reverted, though as mentioned it would really be better IMO to just reset to before the two pointless commits as they are essentially just clutter.

CAM-Gerlach

I honestly have no idea what's going on right now. Now you've reverted your actual good changes too plus added even more pointless revert commits? I don't mean to sound rude, but quite frankly that's exactly the opposite of what I just asked you to do.

Please, just type the following two commands into your git console:

git reset --hard 3206df3
git push -f

which will mercifully undo this mess.

chengtian5huang · 2018-02-09T06:28:59Z

truly sorry for inconvenience.
i will commit one with unit tests added later.

reset as requested

CAM-Gerlach · 2018-02-09T06:33:13Z

i will commit one with unit tests added later.

Thanks

if there are invaild placeholders in template.py, check line by line to replace vaild placeholders and ignore the invailds.

CAM-Gerlach

You have some PEP8 issues you still need to fix; please see the PEP8Speaks comment. Also, you need to rebase it to incorporate recent changes.

CAM-Gerlach · 2018-04-21T22:31:01Z

@chengtian5huang Hey, any updates?

ccordoba12 · 2018-04-23T13:56:26Z

I'm closing this one. I don't understand very well what it tries to fix or improve, sorry.

see issue spyder-ide#6379

ad8ed03

take the part between tripe-double-quotes seperately and deal with it seperately we can replace %(date)s properly no matter what a man add in template.py

jnsebgosselin changed the title ~~see issue #6379~~ Change the way %(date)s and %(username)s are dealt with in the template.py Feb 4, 2018

improved code to fix issue spyder-ide#6379

3206df3

CAM-Gerlach reviewed Feb 9, 2018

View reviewed changes

CAM-Gerlach assigned chengtian5huang Feb 9, 2018

CAM-Gerlach added type:Enhancement component:Editor labels Feb 9, 2018

CAM-Gerlach previously requested changes Feb 9, 2018

View reviewed changes

CAM-Gerlach added the status:Pending Changes/Test label Feb 9, 2018

CAM-Gerlach reviewed Feb 9, 2018

View reviewed changes

CAM-Gerlach previously requested changes Feb 9, 2018

View reviewed changes

chengtian5huang force-pushed the see-issue-#6379 branch from 9e8a87a to 3206df3 Compare February 9, 2018 06:26

chengtian5huang added 3 commits February 9, 2018 15:16

add tests to code for fix#6379

8c764c7

update code to fix issue spyder-ide#6379

1d2d5a6

if there are invaild placeholders in template.py, check line by line to replace vaild placeholders and ignore the invailds.

update to pass travis-ci's and appvoyer's check.

31fec18

CAM-Gerlach changed the title ~~Change the way %(date)s and %(username)s are dealt with in the template.py~~ PR: Change the way %(date)s and %(username)s are dealt with in the template.py Feb 10, 2018

CAM-Gerlach added status:Ready For Review and removed status:Pending Changes/Test labels Mar 11, 2018

CAM-Gerlach added this to the v3.2.x milestone Mar 11, 2018

CAM-Gerlach reviewed Mar 11, 2018

View reviewed changes

CAM-Gerlach added the status:Pending Changes/Test label Mar 11, 2018

CAM-Gerlach removed the status:Ready For Review label Mar 11, 2018

ccordoba12 removed this from the v3.x milestone Apr 23, 2018

ccordoba12 closed this Apr 23, 2018

CAM-Gerlach added resolution:NoResponse and removed status:Pending Changes/Test labels Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Change the way %(date)s and %(username)s are dealt with in the template.py #6380

PR: Change the way %(date)s and %(username)s are dealt with in the template.py #6380

chengtian5huang commented Feb 4, 2018 •

edited by jnsebgosselin

Loading

pep8speaks commented Feb 4, 2018 •

edited

Loading

ccordoba12 commented Feb 4, 2018

chengtian5huang commented Feb 5, 2018

CAM-Gerlach left a comment

CAM-Gerlach Feb 9, 2018

CAM-Gerlach Feb 9, 2018

CAM-Gerlach Feb 9, 2018

chengtian5huang commented Feb 9, 2018 •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach commented Feb 9, 2018

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach left a comment

chengtian5huang commented Feb 9, 2018

CAM-Gerlach commented Feb 9, 2018

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach commented Apr 21, 2018

ccordoba12 commented Apr 23, 2018

PR: Change the way %(date)s and %(username)s are dealt with in the template.py #6380

PR: Change the way %(date)s and %(username)s are dealt with in the template.py #6380

Conversation

chengtian5huang commented Feb 4, 2018 • edited by jnsebgosselin Loading

pep8speaks commented Feb 4, 2018 • edited Loading

Comment last updated on February 10, 2018 at 03:19 Hours UTC

ccordoba12 commented Feb 4, 2018

chengtian5huang commented Feb 5, 2018

i describe in detail how it work under all cases as following:(skip this if u r not interested)

this problem can be divided into following cases

CAM-Gerlach left a comment

Choose a reason for hiding this comment

CAM-Gerlach Feb 9, 2018

Choose a reason for hiding this comment

CAM-Gerlach Feb 9, 2018

Choose a reason for hiding this comment

CAM-Gerlach Feb 9, 2018

Choose a reason for hiding this comment

chengtian5huang commented Feb 9, 2018 • edited Loading

CAM-Gerlach left a comment • edited Loading

Choose a reason for hiding this comment

CAM-Gerlach commented Feb 9, 2018

CAM-Gerlach left a comment • edited Loading

Choose a reason for hiding this comment

CAM-Gerlach left a comment

Choose a reason for hiding this comment

chengtian5huang commented Feb 9, 2018

CAM-Gerlach commented Feb 9, 2018

CAM-Gerlach left a comment • edited Loading

Choose a reason for hiding this comment

CAM-Gerlach commented Apr 21, 2018

ccordoba12 commented Apr 23, 2018

chengtian5huang commented Feb 4, 2018 •

edited by jnsebgosselin

Loading

pep8speaks commented Feb 4, 2018 •

edited

Loading

chengtian5huang commented Feb 9, 2018 •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading

CAM-Gerlach left a comment •

edited

Loading