Could you define work correctly? The path seems to display fine

Sorry, I should be more clear.

We want syntax highlighting and display for review.

Our projects don't use Pull-Request model, rather Patch-Rebase. We submit patches as attachments to the tickets, then we discuss and apply as needed.

As a result, we need a good patch viewer, which we have on trac that does syntax highlighting.

Does that makes sense.

So the issue here is that the mechanism used is there for anything, image, patch, any file. We would need a way figure out if the uploaded document is a patch or a diff or a script or an image, which is not something we have for the moment.

For as long as this feature is not present, I can think of two options:

• Use pull-request and merge manually. You can comment on the PR let the OP adjust, force push to their fork (which will automatically update the PR) and once you're satisfied with the patch, you can download it/them in a single file, edit to add Merges #<id> and push to your repo (the new line will then close the PR specified as merged). This is the workflow used by the ipsilon project.

• The second approach is to use pull-request and let the OP rebase their PR when the work is ready to be merged. The color of the merge button on the PR page indicate if the PR can be merge with a or without a merge commit. This is the workflow I have for pagure, I just ask the PR to be rebased before I merge them keeping the history linear.

I know this isn't quite answering your RFE but maybe they can be way around in the mean time.

Thanks for your input. I'm going to interpret your response as: "We could implement this one day. We don't support it today, and here are some alternatives for now. But we would like to support this in the future".

For now, we aren't in a dire rush to move to pagure, but when we do, this feature will be pretty important for our team, and the work around seems very complex. I'm happy to be patient and wait for this feature. Thanks for your work.

Thanks for your input. I'm going to interpret your response as: "We could implement this one day. We don't support it today, and here are some alternatives for now. But we would like to support this in the future".

That would be correct, just be aware that I have no timeline for this, so unless someone help with this feature, I cannot say when it will land.

Thanks for your input. I'm going to interpret your response as: "We could implement this one day. We don't support it today, and here are some alternatives for now. But we would like to support this in the future".

That would be correct, just be aware that I have no timeline for this, so unless someone help with this feature, I cannot say when it will land.

I've been thinking of allowing people to submit raw git patch files and have Pagure turn it into PRs automatically. Would that help in your case?

Sorry, it wouldn't: My project wants to disable PR's, not have them automatically created. We really just need the patch viewer for code review.

My team (which includes firstyear) is looking at pagure for our fedorahosted trac replacement for 389 Directory Server (as is the freeipa team). This decision for a replacement will be happening very soon. This is actually one of the big draw backs right now as there is no "nice" patch reviewing possible (outside of PRs). Even when you look at commits it does not show all the details/syntax that we are used to in trac: like showing trailing whitespaces, tabs, etc.

Yes, I understand this is a "nice to have" feature, but for us it will weigh heavily on our decision. Even if we can just get it to a point where a file is identified as a patch (which shouldn't be hard), and use the existing pagure commit format display, that would be a significant improvement. Later the syntax highlighting could be added/improved.

Thanks.
Mark

Actually I am working on a patch for this right now. I got it working, but it needs some polishing...

I'm attaching a patch for this issue, as I'm not 100% sure how to do the pull request. Using the UI does not allow me to add new files, and I'm not sure how to remotely connect to my local clone of pagure that's on my laptop. Anyway I attached to patches as described on:

https://docs.pagure.org/pagure/development.html#send-patch

Looks like there is only one patch from you, the other is from me and has been merged in #1523.

Few quick comments:
- the one line description of the commit could be improved: Issue 1382 doesn't say much when you do git log.
- the variable called l could you give it a more descriptive name?
- could you also add a test checking if the content of the file is binary or not? (there is a method is_binary_string that should help)

After that I'll need to test it a little :)

Looks like there is only one patch from you, the other is from me and has been merged in #1523.

I was just following the doc that said to send the previous patch along with the new one (git format-patch -2). Next time I'll only submit the new patch.

Few quick comments:
- the one line description of the commit could be improved: Issue 1382 doesn't say much when you do git log.
- the variable called l could you give it a more descriptive name?
- could you also add a test checking if the content of the file is binary or not? (there is a method is_binary_string that should help)

I will work on all of these shortly. Thanks for the review!!

After that I'll need to test it a little :)

Of course ;)

Thanks again,
Mark

New patch attached. Fixed pep8 errors I found in the files I touched. Also updated some of the test scripts that were impacted by my changes.

New patch attached. Fixed pep8 errors I found in the files I touched. Also updated some of the test scripts that were impacted by my changes.

Just a friendly reminder that this still needs to be reviewed.

Thanks,
Mark

Hm we may want to ping @ryanlerch for this one

-            style="tango",)
+            style="trac")

Since I'm not sure that is something we want to do/change.

Some of the changes here are surprising me, such as

 -            if (flask.g.fas_user.username != comment.user.username
-                    or comment.parent.status != 'Open') \
-                    and not flask.g.repo_admin:
+            if (flask.g.fas_user.username != comment.user.username or
+                    comment.parent.status != 'Open') and not \
+                    flask.g.repo_admin:

Was it a lint tool that asked for this? Most often I prefer to see the logic key word at the beginning of the line so you know you can evaluate the whole line based on it.

Hm we may want to ping @ryanlerch for this one
- style="tango",)
+ style="trac")

Since I'm not sure that is something we want to do/change.

It is impossible to see whitespace errors using tango. Only styles "trac" and "manni" have this ability.

Some of the changes here are surprising me, such as
- if (flask.g.fas_user.username != comment.user.username
- or comment.parent.status != 'Open') \
- and not flask.g.repo_admin:
+ if (flask.g.fas_user.username != comment.user.username or
+ comment.parent.status != 'Open') and not \
+ flask.g.repo_admin:

Was it a lint tool that asked for this? Most often I prefer to see the logic key word at the beginning of the line so you know you can evaluate the whole line based on it.

This change was to fix pep8 errors. I will undo this change if you want.

This change was to fix pep8 errors. I will undo this change if you want.

Was it /usr/bin/pep8 that pointed you to it? If so which version of it are you running? Or did you use something else such as flake8 or pylint or...? :)

This change was to fix pep8 errors. I will undo this change if you want.

Was it /usr/bin/pep8 that pointed you to it? If so which version of it are you running? Or did you use something else such as flake8 or pylint or...? :)

It was only /usr/bin/pep8, it reported errors in the files I touched (not from my changes though). So I fixed them because in pagure docs it said the code should pass pep8. I thought I was doing the right thing. But if this is holding up the review of this patch I will gladly remove those pep8 changes.

So I went back to the pep8 itself http://legacy.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator because my version of /usr/bin/pep8 did not signal this (seems to be because I still run F23 on my main laptop).

Reading pep8 itself, I think I prefer the new style, with the line break before the operator (and I'm going to check the pep8 tool if there is a discussion on this).

Edit: found it! :) https://github.com/PyCQA/pycodestyle/issues/498

Hm we may want to ping @ryanlerch for this one
- style="tango",)
+ style="trac")
Since I'm not sure that is something we want to do/change.

It is impossible to see whitespace errors using tango. Only styles "trac" and "manni" have this ability.

Actually I'm going to work on creating our own pygment Style that uses "tango", but extends it to show the background color of changed lines (so whitespace issues can be detected). Once this is done I'll send it out for feedback.

I attached a screenshot of the new custom diff viewer(its 99.8 % tango, 0.2% trac)

In this screen shot you can see how trailing whitespaces are shown using the background color. In this example they are being removed (in red), but you can see that its at least visual. Tabs are also identified with a special character(double right arrow).

Please review this image and let me know if this is acceptable to move forward with.

@ryanlerch any thoughts on this one? :)

Adding the latest patch

Please review

Thanks,
Mark

the output does look good.

just one note on the colours though, was there any reason for the colourscheme to be different than what is presented in the diffs in the PRs? It would be great if these were the same or at least similar, rather than using a different colour scheme.

Hmmm, it is the same scheme (tango), I "only" added the background color to the changes lines.

This is the exact change to the scheme:

+        Generic.Deleted:           'bg:#ffdddd #a40000',  # trac #000000
+        Generic.Inserted:          'bg:#ddffdd #00A000',  # trac
+        #Generic.Deleted:           "#a40000",        # class: 'gd'
+        #Generic.Inserted:          "#00A000",        # class: 'gi'

The text colour is the same, its only the background colour that was added. I'm fine changing either one (background or text) if you want, but the text colour was not changed with my patch.

@ryanlerch Did you see my comment above? I'd like to know what you want me to do. I'd really like to get this patch in (its been open for quite some time). Thanks!

@mreynolds should we convert this to a PR? I see a few comments I'd do:

• the copyright year in pagure/ui/diff_style.py is incorrect
• I'd like to keep the logical keyword and/or at the start of the line not the end (I think we ran into this one elsewhere)

And it probably needs a rebase with the recent changes we made to decode the content of files.

Otherwise looking good :)

The colours in the latest screenshot are good so 👍from me

Thanks Ryan!

Created pull request:

https://pagure.io/pagure/pull-request/1672

Fixed by @mreynolds in https://pagure.io/pagure/pull-request/1672