new_filename is something that is generated in this block of code. So you are replacing in the content of the comment something that is generating automatically, outside of the comment. How does that work?

The "<!!image>" wasn't the issue, it is merely a place holder so that we know where to place the image/link in the comment (ie: where the user wants the image/link to appear).

What is the idea here?

You may want to read the function pagure.lib.git.add_file_to_git again :)

filelocation seems to be defined within a try/except block, so what happens if an exception was thrown before filelocation was created?

There are quite a few changes in there and some are trivial and some aren't.

It might be an idea to split the trivial changes into another PR to make things easier to review, but I leave that up to you.

I do think some of the changes are un-needed or need to be adjusted as, for example, if you upload a file named test while also mentioning the word test in the comment, I am pretty sure things will not be happy.

@pingou , if a file is uploaded , '\n' will only be added before link if last character is not ' ' or '\n'
if it is image , it automatically views on new line.
Earlier '\n' was always added.

There are quite a few changes in there and some are trivial and some aren't.
It might be an idea to split the trivial changes into another PR to make things easier to review, but I leave that up to you.
I do think some of the changes are un-needed or need to be adjusted as, for example, if you upload a file named test while also mentioning the word test in the comment, I am pretty sure things will not be happy.

working on the changes you suggested :)

All the commits i made in the files, were on the go in the files. So it will be very difficult to split the trivial changes :P . I shall take care next time :)

Ok, thanks :)

@pingou Yes , if there is exception , file shall be removed , :P

Note quite what I was pointed out, see at which line I placed the comment :)

If there is an exception thrown before filelocation creation, there will be database session rollback , during exception handling. Thus no file stored or commited in git :P

There are quite a few changes in there and some are trivial and some aren't.
It might be an idea to split the trivial changes into another PR to make things easier to review, but I leave that up to you.
I do think some of the changes are un-needed or need to be adjusted as, for example, if you upload a file named test while also mentioning the word test in the comment, I am pretty sure things will not be happy.

fixed this, Earlier <!image> was used as placeholder, now as other files in addition to images can be added and later there should be multiple uploading of files (after this pr, i want to :P ), I replaced this by [[name_of_uploaded]] as placeholder.

So you are allowing users to upload images that aren't linked in the comment?

That seems to do something different from what the python code expects

Yes, this is the idea, this gives user choice whether to add link of the file or only use it from attachments.
But (while I am checking now ) attachments method, doesn't seem to allow this at present :P

This is working fine with python code.

This is allowing user to upload files (images or not) without linking them to the comment, I'm not sure we want to do that

The python code seems to expect a placeholder looking like: 1[[filename]] while this is inputting [[ name of the file]] so I am not sure it really works as desired

@pingou , the filename and name_file are same, I tried with various inputs (like init.py , palebo.png, abc (file without extension) ) and seems to work fine on website.

But in tests filename is returned with directory address like /home/vibhcool/Documents/pagure/pagure/tests/placebo.png ,

exploring this bug :)

undoing this change :)

Now this PR solves :-

1) Fix some pep8 errors and spell mistakes (found while reading code)
2) issue #1730 All issue attachments are treated as images

Open for review and tested (not passing tests which are also not being passed by main branch)

Did you consider relying on is_binary_string() instead of mimetypes?

Tests are now passing on master so if you rebase this should work :)

@pingou is_binary_string() doesn't seem to be reliable as it outputs false for all filetypes(tested for py, pyc, png, svg, rst, txt). is_binary() method outputs false for all filetypes except for png. It outputs false for svg file.

That sounds right, they are all text except for png which isn't :)
And since we do not want to render svg anyway, this sounds fine to me :)

@pingou this is new library that got my notice, thank you B-) :)
and rebased :)

This is looking good but I would still prefer moving to is_binary_string()

@pingou Oops,my bad, I misread your comment, working on it :)

tested and up for review :)

I would recommend this change to be different PR altogether.

I am tempting to close this PR, it has not been updated in a year.

Do we want to finish it or shall I close it?

i think we should close it for now. no updates for a year and a 1/4 now.

Closed due to inactivity.

Feel free to reopen or re-file if work on this one re-commences.