Suggest patchname in issue comment

Updated issue summary.

Inserted issue summary template

This is about a patch name suggestion so how about moving it on top.

I clicked the button already so the name is placed to the right. It's behaviour should be the same as from 'Create commit message'

Maybe "Show patch name" is better.

Attached patch is a first stab.

• all lower case (?)
• remove last underscore?
• how many characters to use for the filename?
• input field instead of an window.input?
• should this work for a new issue too?

Please add a new screenshot to the issue summary and a patch to this issue :)

I'm not experienced with sandboxes, can we get a patch posted here?

I was just about to open an issue asking for this and I'm happy to see there is already progress!

the patch in #11 does not seem to have the change to use the pattern mentioned in #6 (it's missing the project name)

For example, for this issue, I get a suggested patch name:
Suggest_filename_for_attachment_in_issue_comment-1294662-12.patch

ha! I see that the patch in 11, used the suggested patch name. :)

oh, sorry, I miss read comment #6. Thanks for the clarification.

can we do some kind of searching and regular expression substring to get it from the breadcrumb?

#1865822-5: Add new button 'Apply' beside existing 'Review' button is getting project name. might be helpful.

Looking good and thanks for adding the helper functions.

I tested this too. it's working well.

Note that I had to commit a follow-up fix for the simplytest.me button, since JS on comment preview pages was completely broken.

But hm. I don't think I agree with this. Most people are using:

[nid]-[cid].patch

I personally prefer to use:

[project].[component]-[facility].[cid].patch

The literal issue title shouldn't be contained either way.

Unfortunately, we're not able to reliably determine the project shortname just on the HTML page output alone. We have JSON representations of issue nodes for quite some time already (e.g., http://drupal.org/node/1294662/project-issue/json), but apparently, that response is not cached at all - i.e., neither on the server, nor on the client-side - so it takes ~600 msecs to deliver that response, which in turn makes it unsuitable for usage in Dreditor.

So in the end, we could probably show a suggestion for [nid]-[cid].patch, but I'm not sure whether that is worth the effort. It's not like anyone would actually care for the name of patch files to begin with...

I think for the patch name, we should go with whatever it says in http://drupal.org/node/1054616
When that changes there, we can change dreditor.

People can still use any filename they want. At least what is in Advanced patch contributor guide is better over many things (which leave out the issue number).

We can open an issue to change what it recommends in the Advanced patch contributor guide.

At this point, I'm rather inclined to mark this issue won't fix — we have no reliable method to retrieve the project name, and the entire issue title definitely does not belong into a patch name; or at least, that should not be suggested as a recommended standard. In turn, that leaves us with the comment number and issue ID only (except that's not available on the issue creation form either) as the only reliable and sensible file name components. Additionally, there's no clarification that the suggested filename is only meant for .patch files, so there's a good chance for the intended novice audience to mistakenly upload interdiffs with the suggested filename, too.

All things combined, this change would add a lot of code (and possibly errors in edge-case scenarios) for a minor feature whose usefulness is questionable.

In case we want to investigate this further nevertheless, then I'd suggest to simplify the implementation and simply base the entire suggested patch filename on the available properties in the issue comment form; i.e., something like this:

var $form = $(context).find('#comment-form');
var suggestion = $form.find('input[name="project_info[project_title]"]').val().replace(' ', '-').toLowerCase()
  + '.' + $form.find('input[name="project_info[component]"]').val().replace(' ', '-').toLowerCase()
  + '.' + Dreditor.Issue.getNid()
  + '-' + Dreditor.Issue.getNewCommentNumber()
  + '.patch';

With that, we'd end up with something alone the lines of this for Drupal core patches:

drupal-core.toolbar.module.123456789-12.patch

Could possibly use some additional component-massaging to strip away ".module" and " system" suffixes (and for contrib projects, we'd want to omit the standard "Code", "User interface", etc components entirely). Could also strip away the " core" suffix for the Drupal project.

Might even make sense to include the [major?] version number after the project (i.e., the first digit of the selected option label of the version field) to clarify whether a patch is against D8 or D7.

At the very least, such suggested filenames would be 100% more sensible and more appropriate.

re-reading the patch doc, it says:
[short-description]
is an 1-2 word summary about what the patch does with dashes between the words.

So, we could
a) limit the suggested patch name to ... 20 chars,
b) use a place holder the human would replace like -short_description-
c) leave it out, since I think the doc recommendation is meant to have a human put the short description there, and dreditor is not a human. so it cannot. :)

How about a) and hope that the humans put meaningful words there?

So, in core, the component is often the module in core, right? Let's use sun's suggestion.

that would yield something like:
drupal-core.toolbar.module-Add_something_to_fix-123456789-12.patch

The "button" is currently labeled "Patchname suggestion", so that might help communicating that it's a suggestion for patches.

We could add something in that pop up window like:

See the Advanced patch contributor guide http://drupal.org/node/1054616 for more details.
So that tests are not run on interdiffs, use the .txt extension, for example: interdiff.txt

Setting to needs work to get sun's concerns addressed.

Note: As soon as you automate something, "all hope is lost."

Therefore, a) and b) will not work out. People will click the button, copy the string, and use it literally.

Cutting the first words will also not work out - lots of issues start with "Allow to perform..." and similar wordings. Patch file names of drupal.form.allow-to-perform.1029404-13.patch would be senseless. ;)

I'd be OK with moving on with a trivial and simple implementation à la #25; i.e., KISS:

[project][major].[component].[nid]-[cid].patch

whereas:

• [project]: Remove " core" suffix for Drupal project. And .replace(/[^a-zA-Z0-9]+/, '-').toLowerCase().
• [major]: First digit of Version dropdown option label, which always means the core compatibility; i.e., .substring(1).
• [component]: Ignore values "Code", "User interface", "Miscellaneous". Replace "Documentation" with "docs". Remove ".module" and " system" suffixes. And .replace(/[^a-zA-Z0-9]+/, '-').toLowerCase().
• [nid]: Issue ID.
• [cid]: Predicted number of next comment.

I started fixing some comment issues. But then I realized that this is broke in some ways.

when #28 is applied, the Create commit message does not work (it jumps to the beginning of the page).
Also, it does not work on core issues like http://drupal.org/node/1917212, for example, the patchname suggestion does not work there either.

I fixed the comments anyway.
I think it makes sense to take the title out of contrib patch suggestions too, for the same reasons as leaving it out from core ones.
I put a dot in as a divider between the component and the issue number... since the component has a dash in it, dash for the separator there is confusing.
We could change the component - to be _ ... but I dont think that's really the way to go.

Someone else will have to figure out the still broken parts, as I dont know how.

the version is not working..
for example in #1294662: Suggest patchname in issue comment
drupal.x.language-system.365615-141.patch

Also, I dont know how careful we want to be with comments but...

+++ b/dreditor.user.jsundefined
@@ -1579,6 +1574,201 @@ Drupal.behaviors.dreditorFormBackup = function (context) {
+/*
+ * Gets the issue node id.
+ */

beginning of comment block uses two stars like..

/**
 * @defgroup jquery_extensions jQuery extensions
 * @{
 */

this happens several places in this patch.

+++ b/dreditor.user.jsundefined
@@ -1579,6 +1574,201 @@ Drupal.behaviors.dreditorFormBackup = function (context) {
+ * Gets the selected version

Also, some sentences in a few places are missing period at the end.

Also, if you name the interdiff something like interdiff-X-Y.txt, then it will not get the simplytest.me button.. applying an interdiff to test does not really make sense. :)

those are nits, small things.
setting needs work for the version number for core though.

i did a git clone of dreditor and saved the patch from #30. Then I did a git apply and got error patch failed dreditor.user.js:717

I am going to follow these instructions to reroll, http://drupal.org/patch/reroll.

I have tried to reroll this patch with these instructions, http://drupal.org/patch/reroll and have found that the last two patched #29 and #30 did not apply.

YesCT and I are confused.

The patches in #29 and #30 applied cleanly at 590d2ed, but there was a merge conflict with code committed later (probably febe205e).

• dreditor.user-interface.1294662-34-reroll.patch is a straight rebase ours-full
• dreditor.user-interface.1294662-34.patch moves new code from HEAD to the new Drupal.dreditor.issue.getProjectShortName() method. We also rephrased the comments a bit.

These still need manual testing.

Manual testing:
all my dreditor goodness is gone.

+++ b/dreditor.user.jsundefined
@@ -1585,6 +1582,208 @@ Drupal.behaviors.dreditorFormBackup = function (context) {
+      // Truncate and remove a heading/trailing undescore

small fix to add punctuation to sentence.

also an extra newline removed.

here is an updated screenshot

here is what one looks like on core:
drupal8.configuration-system.1653026-52.patch

And when I change the value of the version (and/or component) in the dropdowns on the comment, the patch name suggestion changes to match too.
drupal7.configuration-system.1653026-52.patch

rtbc since my changes are two small comment fixes.

Maybe a follow-up to add a link to more info on patch names?
http://drupal.org/node/1054616
(mentioned in #26)
which can say See http://drupal.org/node/1054616 for more information on patch naming conventions including interdiff.txt and do-not-test.patch

@sun I think this addressed your previous concerns. Anything that needs work here?
Would be helpful to get this in before the Get Involved Sprint at portland. :)

Just tested and works great! Woohoo :) RTBC++

I just checked, it applies with hunk offset ok, so dont need a reroll.
and it still works fine.

:)

Fixed in commit 17bcb89.