dawnbeen / c_formatter_42

C language formatter for 42 norminette
GNU General Public License v3.0
176 stars 17 forks source link

c_formatter_42 does not follow include statements with a new line #38

Closed kmbenjel closed 1 year ago

kmbenjel commented 2 years ago

I tried with the usual header includes and with the sort of #include "libft.h"

When I don't add a new line there accordingly to the Norm, it does not auto-fix either it is the sole thing or among other things fixed in the same run.

This is in VScode, tried it with and without the Vim extension, and It did not work in Vim on ZSH as well.

I'm starring the repo, thanks for the useful extension, and I'll be always there for feedback.

Best, Khalid (1337 Benguerir)

cacharle commented 2 years ago

Hi, thx for the feedback.

What do you mean by not adding a newline? any examples?

from what I understand you have:

#ifndef ...
#define ...

#include "libft.h"#include <stdio.h>

#endif

probably not what you have, but its what i understand from your issue.

kmbenjel commented 2 years ago

Thank you, No, I have:

#include "libft.h"
void *ft_function(char c)

The intended fix is:

#include "libft.h"

void *ft_function(char c)

I expressed wrongfully, that Norminette as well is not precise in this, I should say it's Norminette's fault.

The Norm V3 says:

Any comment or preprocessor instruction can be right above the function.The newline is after the previous function.

The French version says the same, but Norminette says: Preprocessor statement must be followed by a newline = wanting you really to add an extra new line and have an empty line after the preprocessor statement!

So, you are urged to add that blank line to get the Norminette OK.

cacharle commented 2 years ago

aah, indeed that makes more sense.

Unfortunately, I don't have time to fix this.

But it shouldnt be too hard to implement in the preprocessor formatter https://github.com/dawnbeen/c_formatter_42/blob/cf0738d3068f309f346e0a99bf6ca4ccf9ba8259/c_formatter_42/formatters/preprocessor_directive.py#L16

Or create a new formatter based on this one.

I also feel like this isnt a big hassle to fix by hand so if you dont have time to fix this issue either, I'll close it

kmbenjel commented 2 years ago

Useful, thanks. let it open if you wish, I will try to fix it and make a PR. It could be fairly late though. Sure it's easy to handle, I just tend to be helpful to the coming users. I hope new collaborators come in in the future to keep maintaining this lovely plugin, though it's already close to perfect.

kmbenjel commented 2 years ago

I did:

return "\n".join(lines) + '\n'

It worked.

kmbenjel commented 2 years ago

I have to tweak more, cause it added a blank line after the function as well.

kmbenjel commented 2 years ago

Sorry for bothering you a lot :) Copilot gave me his feedback, this edit worked:

        if directive_name == "endif":
            indent = 0

If there will be no complicated thing to do, I could open up a PR.

cacharle commented 2 years ago

dont add the copilot code, it would break with nested if directives.

for the PR to be merged, you have to write some unit tests.

kmbenjel commented 2 years ago

Ok, About Copilot, If it's about the if directives inside the file, don't worry, I edited accordingly, I changed -1 by 0 in that nested statement and that's it. I did not just put Copilot's code among the code. About the unit test, I'll check my ability to do so. I will also ask @younesaassila if he could kindly take care of it, he would be proficient in writing unit tests and the task seem fairly easy.

Thank you for your time.

kmbenjel commented 2 years ago

Hi, Sorry, It was all wrong. I studied more, now I added the two lines that I'm commenting on in the snippet below. It fixes when you omit a blank line after the last include statement, but It makes two blank lines when you have initially a blank line or more.

I hope someone gets the initiative to make a PR with a complete and well-tested solution when they get some time. At the same time, I opened an issue in the Norminette repo about the discussed matter.

import re

def preprocessor_directive(content: str) -> str:
    lines = content.split("\n")

    directive_regex = r"^\#\s*(?P<name>[a-z]+)\s?(?P<rest>.*)$"
    matches = [re.match(directive_regex, line) for line in lines]
    idented = [(i, match.group("name"), match.group("rest"))
               for i, match in enumerate(matches)
               if match is not None]
    indent = 0
    for i, directive_name, directive_rest in idented:
        if directive_name in ["elif", "else", "endif"]:
            indent -= 1
        lines[i] = "#" + (" " * indent) + directive_name + " " + directive_rest
        lines[i] = lines[i].strip()
        #if directive_name == "include" and i + 1 < len(lines) and re.match(directive_regex,
        #lines[i + 1]) is None:
            #lines[i] += '\n'
        if directive_name in ["if", "ifdef", "ifndef", "elif", "else", "endif"]:
            indent += 1
        if directive_name == "endif":
            indent = -1
    return "\n".join(lines)
cacharle commented 2 years ago

looks good but i'm not sure what happens if you run the formatter multiple times (would it add one more newline each time?) or what if the user has other directives between the includes?

kmbenjel commented 1 year ago

No, I figured out right after that, that it's not good at all, yet. It fixes for the first time when no blank line, afterwards it remains consistent on two blank lines, not one. :)))) I kept it commented out, probably I'll work on It seriously whenever I get some time. I think it's an interesting thing to fix for consistency.