Page MenuHomePhabricator
Create Task
Maniphest T152996

newitem.py -touch crashes on locked pages
Closed, ResolvedPublic
Actions

Description

pwb.py newitem -file:aa.txt -lang:cs -touch

...

[[cs:Šablona:Anontools]] already has an item: [[wikidata:Q6023401]].
Doing a null edit on the page.
WARNING: API error protectedpage: The "editprotected" right is required to edit this page
ERROR: Page [[cs:Šablona:Anontools]] is locked.
Traceback (most recent call last):
  File "I:\pwb\pywikibot\bot.py", line 1948, in run
    self.treat(page, item)
  File ".\scripts\newitem.py", line 71, in treat
    page.put(page.text)
  File "I:\pwb\pywikibot\tools\__init__.py", line 1447, in wrapper
    return obj(*__args, **__kw)
  File "I:\pwb\pywikibot\page.py", line 1292, in put
    **kwargs)
  File "I:\pwb\pywikibot\tools\__init__.py", line 1447, in wrapper
    return obj(*__args, **__kw)
  File "I:\pwb\pywikibot\page.py", line 1209, in save
    cc=apply_cosmetic_changes, quiet=quiet, **kwargs)
  File "I:\pwb\pywikibot\page.py", line 1234, in _save
    raise err
LockedPage: Page [[cs:Ĺ ablona:Anontools]] is locked.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
newitem.py doesn't crash on LockedPagepywikibot/coremaster+29 -12
Customize query in gerrit

Event Timeline

JAnD created this task.Dec 12 2016, 7:32 PM
Restricted Application added subscribers: pywikibot-bugs-list, Aklapper. · View Herald TranscriptDec 12 2016, 7:32 PM
Magul subscribed.Dec 12 2016, 11:52 PM

Ok, and what do You think, should be expected behaviour here?

JAnD added a comment.Dec 13 2016, 7:28 PM
In T152996#2866783, @Magul wrote:

Ok, and what do You think, should be expected behaviour here?

Expected is skipping page rather than crash

When touch.py finds locked page, simply skips:

...
Page [[Šablona:Colosseum]] saved
WARNING: API error protectedpage: The "editprotected" right is required to edit this page
ERROR: Page [[Šablona:Commons]] is locked.
Page [[Šablona:Commons/doc]] saved
Page [[Šablona:Commonscat]] saved
Page [[Šablona:Commonscat/doc]] saved
...
Magul claimed this task.Dec 16 2016, 3:02 PM
gerritbot subscribed.Dec 16 2016, 3:09 PM

Change 327740 had a related patch set uploaded (by Magul):
newitem.py doesn't crash on LockedPage

https://gerrit.wikimedia.org/r/327740

gerritbot added a project: Patch-For-Review.Dec 16 2016, 3:09 PM
Magul added subscribers: Xqt, Dalba.Dec 18 2016, 11:32 AM

@Dalba , @Xqt could check this review? There is a question about 79-characters limitation - do we want to always stick to it or do we aproach this limitation in more liberal way?

Dalba added a comment.EditedDec 18 2016, 11:57 AM

I usually recommend keeping lines below 80 characters, but only for those lines that are newly added. Older lines are usually kept unchanged which I assume helps when using git blame or searching the git log for specific change.

But personally I feel much more comfortable with the 80 characters line limit and would vote to merge changes like https://gerrit.wikimedia.org/r/#/c/287412/ .

Magul added a subscriber: Mpaa.Dec 18 2016, 12:24 PM
Lokal_Profil subscribed.Dec 20 2016, 10:00 PM
In T152996#2884917, @Dalba wrote:

I usually recommend keeping lines below 80 characters, but only for those lines that are newly added. Older lines are usually kept unchanged which I assume helps when using git blame or searching the git log for specific change.

But personally I feel much more comfortable with the 80 characters line limit and would vote to merge changes like https://gerrit.wikimedia.org/r/#/c/287412/ .

I'm happy to merge as is. Consistently applying the 80 char limit trumps the minor aesthetic loss in this particular case.
One work-around would be to go for something like:

self.pageAgeBefore = self.repo.getcurrenttime() - \
                     timedelta(days=self.pageAge)

which is slightly clearer but has it's own aesthetic losses.

Magul added a comment.Dec 23 2016, 8:07 PM
In T152996#2891272, @Lokal_Profil wrote:

I'm happy to merge as is. Consistently applying the 80 char limit trumps the minor aesthetic loss in this particular case.

Do I need to do something here? Or do You lose this patch from sight after re-check? @Lokal_Profil ?

Lokal_Profil added a comment.Dec 27 2016, 9:05 PM
In T152996#2899565, @Magul wrote:
In T152996#2891272, @Lokal_Profil wrote:

I'm happy to merge as is. Consistently applying the 80 char limit trumps the minor aesthetic loss in this particular case.

Do I need to do something here? Or do You lose this patch from sight after re-check? @Lokal_Profil ?

I was awaiting a resolution here about the line breaking. I notice that Dalby has found some minor issues with the patch though.

Magul added a comment.Dec 29 2016, 9:10 AM

@Lokal_Profil and where is this @Dalba commnet? I cannot see it here or on gerrit.

Lokal_Profil added a comment.EditedDec 29 2016, 9:46 PM

My mistake I looked at the other commitments mentioned in this thread.

gerritbot added a comment.Dec 29 2016, 9:49 PM

Change 327740 merged by jenkins-bot:
newitem.py doesn't crash on LockedPage

https://gerrit.wikimedia.org/r/327740

Magul closed this task as Resolved.Jan 1 2017, 12:55 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits