Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[[file:img|12px|alt=words]] and such #783

Merged
merged 13 commits into from
Sep 20, 2018
Merged

[[file:img|12px|alt=words]] and such #783

merged 13 commits into from
Sep 20, 2018

Conversation

GlazerMann
Copy link
Collaborator

@GlazerMann GlazerMann commented Sep 16, 2018

Current code does [[]] and [[|]].
Added up to this size:
[[File:Example.png|thumb|upright|alt=Example alt text|Example caption]]

@GlazerMann GlazerMann changed the title Add test for brackets brackets -needs fixed Sep 16, 2018
@GlazerMann GlazerMann changed the title brackets -needs fixed [[file:img|12px|alt=words]] in titles Sep 16, 2018
@GlazerMann GlazerMann changed the title [[file:img|12px|alt=words]] in titles [[file:img|12px|alt=words]] and such Sep 16, 2018
Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Two thoughts:

(i): There's gotta be a better way! A recursive preg_replace, perhaps: while($text = preg_replace($START.$MID.$END)){} or something?

(ii): The test cases do not look like examples that would be encountered in the real world. Surely no-one should ever include an image in a title field. I don't think we need to worry about handling every garbage in, garbage out situation: if it's not a use case listed in the Citation template documentation, then it's the user's problem, not ours.

@josve05a
Copy link
Contributor

It might be "garbage in", but it is valid garbage which renders properly. The bot currently makes it worse. And it is currently present on over 600 articles (insource:/title=[[([Ff]ile|[Ii]mage):/ -- note that a few are not cite templaes though)

@ms609
Copy link
Owner

ms609 commented Sep 18, 2018

By making it worse, the bot may help to draw attention to the incorrect information, increasing the chance that a human editor will step in and clean up the mess.

If it is a recurrent problem, then perhaps the cite templates can be modified to notice the bad input and notify users more prominently. That way users will gain immediate feedback and be able to change their behaviour, rather than the bot stepping in months later to make an edit that the original user may not see.

@josve05a
Copy link
Contributor

josve05a commented Sep 18, 2018

If a "bodge" is incorrect but works, it should be "noted" with a CS1|2 error/maint. message rather than having the bot make the "bodge" both incorrect and not work. Rather have an error incorrect but "look nice" than incorrect and visually broken imo. There are more ways to draw attention to problems (such as maint. messages/categories/templates/etc.) than breaking a working template.

@GlazerMann
Copy link
Collaborator Author

The example is real-world.

@ms609
Copy link
Owner

ms609 commented Sep 18, 2018

Yes, I agree that a CS1/2 error message could be useful here, particularly given how widespread you have found the misuse to be!

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #783 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #783      +/-   ##
===========================================
+ Coverage     70.15%   70.2%   +0.04%     
- Complexity     1649    1650       +1     
===========================================
  Files            12      12              
  Lines          3334    3339       +5     
===========================================
+ Hits           2339    2344       +5     
  Misses          995     995
Impacted Files Coverage Δ Complexity Δ
Template.php 76.49% <100%> (+0.05%) 1470 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171a4c8...4fc77fc. Read the comment docs.

@GlazerMann
Copy link
Collaborator Author

@ms609 it is now done in a loop.

Copy link
Owner

@ms609 ms609 left a comment

Choose a reason for hiding this comment

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

Much better, thanks!

@ms609 ms609 merged commit 3e533ca into master Sep 20, 2018
@ms609 ms609 deleted the GlazerMann-patch-6 branch September 20, 2018 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants