remove indentation here, should be

"""
Ensure that the ...
"""

Ok 👍

just check for #!/usr/bin/env

having #! in the first line don't necessarily measn that it was intended to be the shebang operator

so you just add the shebangs if #!/usr/bin/env it is not found

or what do you think?

The referenced issue requires that coala enforces using the #!usr/bin/env (#1932).
Additionally, I am not familiar with the unix flags so i don't know whether #! can be used for purposes other than the shebang operator.

A shebang basically specifies the shell for the command to call the interpreter (like python, ruby, php, or another UNIX shell). We want to enforce the usage of using /usr/bin/env to get the proper path of the interpreter.

@yukiisbored so first of all, the bear should be moved from the python bears directory.
Now, let's discuss the case where the user uses something like #!/usr/bin/python , how can we check that the user doesn't use the general form of the shebang operator and then deduce that script is written in python(or any other language) ?
Do you think that the current check can be improved somehow?
Thanks 😃

@AMR-KELEG You can just grab the binary name (without the path) and add that at the end of /usr/bin/env .

ex.

1. #!/usr/bin/python -> #!/usr/bin/env python
2. #!/usr/bin/python3 -> #!/usr/bin/env python3
3. #!/usr/bin/asuka --intepreter-mode --no-warnings -> #!/usr/bin/env asuka --intepreter-mode --no-warnings

Also, I think the fitting category is between shell and general (depends on how you look at it ;))

and tests for this bear

you don't want #!/usr/bin/env as your shebang without any binary right? That's just a directory

Actually, afaik env is used to run a program inside a modified environment (from env(1) GNU coreutils).

@sils Yes, you're right.
The modified line should include the binary.
#!/usr/bin/env isn't enough.
The modified line should look like: #!/usr/bin/env python.
@yukiisbored What do you think?

@AMR-KELEG It's ok. Please don't wait for our response just do the changes needed. We're pretty busy with a lot of stuff and we don't want to have this PR delayed just because of us~

I assume this is message, use message or msg or something that is more clear as a message

Ok

use 'Use /usr/bin/env {}'.format(binary) instead ?

Can you please use os.path.basename instead of this? Since, it causes problems if it's /usr/sbin/, /sbin/, /usr/local/sbin, etc.

I also noticed an empty list is False and I think it's better to do it that way.
https://stackoverflow.com/questions/53513/best-way-to-check-if-a-list-is-empty#53522

I think .startswith('#!/usr/bin/env') is better...

I find this message to be unclear especially for users who don't know what is /usr/bin/env. Any ideas? I'm not sure how to phrase it as well.

"Use /usr/bin/env to call the program" and then a more info "/usr/bin/env is preferable to be used because blah blah blah ..." is better I think...

Same as https://github.com/coala/coala-bears/pull/1937/files#r129461801 but just check for #!.

I find both of the above to be tricky when reading it. I think it's better to put it inside the if statement below and just yield it inside it as well.

Basically,

if "check if it starts with a shebang":
    diff = Diff(file)
    ...
    message = "message goes here"
    ...
    yield ...

Maybe we should also return a result if the file is empty, saying there's no shebang?

Maybe without a patch, but at least saying it's missing ;)
(oh would be so cool if we would have custom result actions, then we could write one where the user can input immediately the name of the binary... but we don't have it yet 😢)

Could you also add a newline here? I think it improves readability 👍

I think you are using #!/usr/bin/env quite often, I think it's worth to put it inside a variable as a constant 👍

It is only written twice here..
Putting it inside a variable would be only beneficial if we will need to change it later..

it's also for consistency 👍
Everything that's used more than once shouldn't be inlined ;)

don't you have to strip off the #! part in file[0]?
So in the end you have file[0][2:]?

I don't know if i understood your comment but the basename extracts the binary file out of the path.
https://docs.python.org/2/library/os.path.html#os.path.basename
Why would i strip the #! if i don't care what the first part of the path is?

but is os.path.basename able to handle it? As the path starts after #!

https://github.com/python/cpython/blob/3.6/Lib/posixpath.py#L142
The method just extracts all the characters after the last separator.
IMO, we shouldn't strip off the #!.
What do you think?

Those characters don't belong to the path, that's why I'm asking for it, as conceptually it's not "completely right". I would do file[0][2:] for the sake of comprehensibility ;)

Hmm I'm not that convinced of the message, but can't think of a better one right now... we'll discuss it again ;)

But please inline message_desc, I think it's just an unnecessary extra variable ;) Then you don't have to use two placeholders ;)

I think you can be precise and describe the idea of using /usr/bin/env ;)

Can you also assert for the diff? :)

I couldn't figure out how to assert it.
https://github.com/coala/coala/blob/master/coalib/results/Result.py#L161

@Makman2 could you please give me a hint?
Thanks 😄

just use the diff param in the result here 👍

I think an empty file should be written as an empty list, that's how python handles it 👍

Maybe you should put those into test files too? Up to you :)

Hmm not sure if it's syntax... do we have a better fitting keyword?

Formatting?
I think syntax is the better choice.

You aren't correcting syntax errors... hmm... Could be that you have to create a new keyword, maybe Shebang itself works?
You could maybe get some ideas from the community ;)

please add a newline here to separate system imports from others ;)