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

Rmtr order #1735

Merged
merged 5 commits into from Aug 10, 2023
Merged

Rmtr order #1735

merged 5 commits into from Aug 10, 2023

Conversation

Trialpearswiki
Copy link
Contributor

Closes #1718. Simply a regex change to place it above the header below. If you change Tested here. The regex may seem more complicated than necessary, but it's required to avoid spacing issues if we have extra newlines which were tested in the two edits before the linked one.

@siddharthvp
Copy link
Member

The comments on RMTR still indicate that requests should be placed at the top of the section rather than bottom.
Screenshot 2023-04-29 at 5 04 56 PM

@NovemLinguae
Copy link
Member

  • Re SD0001: I think the HTML comment is not clear about where it wants the new entry. I'd be OK with leaving the comment alone for now. I don't think it needs to be a blocker for this patch.
  • I think this patch could benefit from adding a newline after new entries and before the "Controversial technical requests" header. Right now it squeezes everything together.
    image
  • I think any situation where you're using regexes or transforming wikicode are great candidates for a string in, string out function and unit tests. Saves a lot of manual testing.

I may code up bullets #2 and #3 if I get time today.

@NovemLinguae
Copy link
Member

NovemLinguae commented May 20, 2023

I tried adding unit tests, looks hard. The code that makes the wikitext is tightly coupled to Morebits in a subfunction. Giving up on that for now.

Will revisit this later. If the \n issue (bullet number 2) can be easily solved, would appreciate a patchset for that.

@NovemLinguae
Copy link
Member

Hey @siddharthvp, can you take a look at the code in this one when you get a moment? I added some unit tests.

@NovemLinguae NovemLinguae merged commit 7133c38 into wikimedia-gadgets:master Aug 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RM/TR addition order
3 participants