PR: Limit the number of lines in history according to the relevant setting #7132

impact27 · 2018-05-15T07:04:56Z

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
Wrote at least one-line docstrings for any new functions
Added at least one unit test covering the changes, if at all possible
Described your changes and the motivation for them below
Noted what issue(s) this pull request resolves, creating one if needed
~~Included a screenshot, if this PR makes any visible changes to the UI~~

Description of Changes

A better thing would be to read the history from ipython, but I am not sure that this would work in every cases.

Issue(s) Resolved

Fixes #7080

A better thing would be to read the history from ipython, but I am not sure that this would work in every cases.

pep8speaks · 2018-05-15T07:04:58Z

Hello @impact27! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on June 15, 2018 at 22:19 Hours UTC

CAM-Gerlach · 2018-05-15T14:22:14Z

spyder/plugins/history.py

+        linebreaks = [m.start() for m in re.finditer('\n', text)]
+        maxNline = CONF.get('historylog', 'max_entries')
+        if len(linebreak) > maxNline:
+            text = text[linebreaks[-maxNline] +1:]


Whitespace around +

CAM-Gerlach · 2018-05-15T14:27:30Z

Thanks for doing this! I need to head to work now, but a few quick things:

Please briefly describe what your PR actually does in the title and the description, so we can follow it easily
Also, thanks for including the Fixes # message in your commit, but it's also good to include it in the body of the PR, which the template explicitly has a line for
~~Fix the PEP8 issue~~
The test failures are legitimate, and it looks like its due to a typo in your code (see comment). It's easy to make, but if you're using Spyder or had actually tested the change, it would have been easy to catch
Can you add a test for this?
Not sure why you deleted the PR template that specified several of these things in a checklist...(I added it back for your reference)

CAM-Gerlach

Appears to be a typo bug causing all the tests to fail. I'll actually test this later once you've fixed your bug, etc, but please describe what this is intended to do and how so I can most easily evaluate it. Thanks!

CAM-Gerlach · 2018-05-15T14:30:08Z

spyder/plugins/history.py

@@ -230,6 +232,11 @@ def add_history(self, filename):
        editor.toggle_wrap_mode( self.get_option('wrap') )

        text, _ = encoding.read(filename)
+        linebreaks = [m.start() for m in re.finditer('\n', text)]
+        maxNline = CONF.get('historylog', 'max_entries')
+        if len(linebreak) > maxNline:


This is where all the tests are failing--it should be linebreaks presumably. Easy mistake to make, but one a basic code checker or a simple test (unit test or just actually testing that the functionality worked without error) would have caught.

All points addressed. Thanks!

ccordoba12 · 2018-05-15T23:00:27Z

spyder/plugins/history.py

@@ -29,6 +30,7 @@
 from spyder.widgets.tabs import Tabs
 from spyder.widgets.sourcecode import codeeditor
 from spyder.widgets.findreplace import FindReplace
+from spyder.config.main import CONF


Please remove this. It's not needed.

ccordoba12 · 2018-05-15T23:01:51Z

spyder/plugins/history.py

@@ -230,6 +232,11 @@ def add_history(self, filename):
        editor.toggle_wrap_mode( self.get_option('wrap') )

        text, _ = encoding.read(filename)
+        linebreaks = [m.start() for m in re.finditer('\n', text)]
+        maxNline = CONF.get('historylog', 'max_entries')


Plugins can access config their own options like this

maxNline = self.get_option('max_entries')

ccordoba12 · 2018-05-15T23:02:33Z

Things look good, I just left a minor comment.

CAM-Gerlach · 2018-05-15T23:04:37Z

@ccordoba12 Do we want a regression test for this to ensure the change is and remains effective?

ccordoba12 · 2018-05-15T23:13:06Z

A test would be nice, yes, but it doesn't seem easy to implement. Besides, the code looks good.

CAM-Gerlach · 2018-05-15T23:23:00Z

Besides, the code looks good.

Well, those are some famous last words if I ever heard some, heh... :)

ccordoba12

I made myself the changes I requested. They were very simple.

ccordoba12 · 2018-05-26T18:09:11Z

spyder/plugins/history.py

@@ -230,6 +231,11 @@ def add_history(self, filename):
        editor.toggle_wrap_mode( self.get_option('wrap') )

        text, _ = encoding.read(filename)
+        linebreaks = [m.start() for m in re.finditer('\n', text)]


We need to convert all EOL's to \n before doing this.

Can't we just always use LF for our own history files, independent of the platform?

…gle one

Fixes #7080

Fixes spyder-ide#7080

3c12105

A better thing would be to read the history from ipython, but I am not sure that this would work in every cases.

CAM-Gerlach reviewed May 15, 2018

View reviewed changes

CAM-Gerlach assigned impact27 May 15, 2018

CAM-Gerlach added type:Bug component:History Log labels May 15, 2018

CAM-Gerlach added this to the v3.3 milestone May 15, 2018

CAM-Gerlach changed the title ~~Fixes #7080~~ PR: Fixes #7080 May 15, 2018

CAM-Gerlach previously requested changes May 15, 2018

View reviewed changes

pep8

922e0f7

impact27 changed the title ~~PR: Fixes #7080~~ PR: Limit the number on lines in the history according to the relevant setting. May 15, 2018

Correct a typo

788993f

ccordoba12 reviewed May 15, 2018

View reviewed changes

CAM-Gerlach changed the title ~~PR: Limit the number on lines in the history according to the relevant setting.~~ PR: Limit the number of lines in the history according to the relevant setting. May 15, 2018

Don't use CONF because it's not needed

b58c529

ccordoba12 approved these changes May 26, 2018

View reviewed changes

ccordoba12 reviewed May 26, 2018

View reviewed changes

ccordoba12 added 4 commits June 15, 2018 16:24

utils/sourcecode: Add a function to normalize eols in a text to a sin…

0c4b215

…gle one

History: Normalize eols before trying to detect how many lines are on it

a777927

History: Really preserve max_entries when cutting history

12e8cd3

Testing: Add max_entries test for history

2a39855

ccordoba12 changed the title ~~PR: Limit the number of lines in the history according to the relevant setting.~~ PR: Limit the number of lines in history according to the relevant setting Jun 15, 2018

ccordoba12 merged commit 40b430d into spyder-ide:3.x Jun 15, 2018

ccordoba12 added a commit that referenced this pull request Jun 15, 2018

Merge from 3.x: PR #7132

ef7f1ff

Fixes #7080

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Limit the number of lines in history according to the relevant setting #7132

PR: Limit the number of lines in history according to the relevant setting #7132

impact27 commented May 15, 2018 •

edited by ccordoba12

Loading

pep8speaks commented May 15, 2018 •

edited

Loading

CAM-Gerlach May 15, 2018

CAM-Gerlach commented May 15, 2018 •

edited

Loading

CAM-Gerlach left a comment

CAM-Gerlach May 15, 2018

ccordoba12 May 15, 2018

ccordoba12 May 15, 2018

ccordoba12 commented May 15, 2018

CAM-Gerlach commented May 15, 2018

ccordoba12 commented May 15, 2018

CAM-Gerlach commented May 15, 2018

ccordoba12 left a comment

ccordoba12 May 26, 2018

CAM-Gerlach May 26, 2018 •

edited

Loading

PR: Limit the number of lines in history according to the relevant setting #7132

PR: Limit the number of lines in history according to the relevant setting #7132

Conversation

impact27 commented May 15, 2018 • edited by ccordoba12 Loading

Pull Request Checklist

Description of Changes

Issue(s) Resolved

pep8speaks commented May 15, 2018 • edited Loading

Comment last updated on June 15, 2018 at 22:19 Hours UTC

CAM-Gerlach May 15, 2018

Choose a reason for hiding this comment

CAM-Gerlach commented May 15, 2018 • edited Loading

CAM-Gerlach left a comment

Choose a reason for hiding this comment

CAM-Gerlach May 15, 2018

Choose a reason for hiding this comment

ccordoba12 May 15, 2018

Choose a reason for hiding this comment

ccordoba12 May 15, 2018

Choose a reason for hiding this comment

ccordoba12 commented May 15, 2018

CAM-Gerlach commented May 15, 2018

ccordoba12 commented May 15, 2018

CAM-Gerlach commented May 15, 2018

ccordoba12 left a comment

Choose a reason for hiding this comment

ccordoba12 May 26, 2018

Choose a reason for hiding this comment

CAM-Gerlach May 26, 2018 • edited Loading

Choose a reason for hiding this comment

impact27 commented May 15, 2018 •

edited by ccordoba12

Loading

pep8speaks commented May 15, 2018 •

edited

Loading

CAM-Gerlach commented May 15, 2018 •

edited

Loading

CAM-Gerlach May 26, 2018 •

edited

Loading