Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Option to work with .gitignore to perform parsing on files that don't match. #416

Closed
kiokuless opened this issue Jul 30, 2023 · 3 comments · Fixed by #417
Closed
Labels
enhancement New feature or request

Comments

@kiokuless
Copy link
Contributor

kiokuless commented Jul 30, 2023

Hi, I swam the Piscine in July and used this parser so much! I always wrote the following .gitignore to avoid submitting my main.c or a.out.

# Ignore all, contains .gitignore itself
/*
# Do not ignore files to be submitted
!/ex[0-9][0-9]/ft_*.c

However norminette parses main.c and produces useless output. It would be so cool to have an option that would fulfill the title requirement! My Piscine is finished and I am free to work on this feature, can submit PR.

To add parsing .gitignore, we will choose one of the following two approaches

  • Add a dependency to mherrmann/gitignore_parser. This is a minimal lirary that doesn't depend on any other PyPI libraries and looks great!
  • Write our own .gitignore parser

I hope you like this feature proposal!

@NiumXp
Copy link
Contributor

NiumXp commented Jul 31, 2023

Hello, this is a feature request similar to #257 that was rejected.

By the way, instead of adding an external library to norminette, why not use git with the built-in module subprocess? Using it can avoid some issues, like you wrote in PR about recursive paths.

import subprocess


# all_files is already collected by norminette
all_files = {"ft_putchar.c", "ft_putstring.c", "main.c", "test.c"}
try:
    command = f"git check-ignore {' '.join(all_files)}"
    routput = subprocess.check_output(command, shell=True, universal_newlines=True)
except subprocess.CalledProcessError:
    ...
else:
    ignored_files = set(routput.split())
    # Now we can send to norminette the files that can be checked
    checkable_files = all_files - ignored_files

@kiokuless
Copy link
Contributor Author

kiokuless commented Jul 31, 2023

Hello @NiumXp ! Your suggestion for using subprocess is a better way definitely! I'll update my PR. Also, thanks for sharing the past PR! I hadn't looked through it and they are very helpful.

My opinion is a bit different, in the first place there should not be unnecessary files such as .gitignore or .norminetteignore in the repository at the time of evaluation. I find it strange that reviewers use this option. Or if reviewers use this option for a repository at submission, the ".gitignore is missing!" error should appear. Sorry for the extreme example, but if #257 stands, the reviewer's cc command should always be forced to be aliased cc -Wall -Wextra -Werror.

I can't say anything since my admissions results are not yet available and I haven't seen the main student's assignment, but I don't believe this is an option that can be abused. It seems to me that the only reviews where this would be abused are reviews that have underlying problems in other areas.

I hope you will reconsider, and if you still disagree to this option, you can close this Issue without worrying about me. I really appreciate your opinion! Thank you!

@NiumXp
Copy link
Contributor

NiumXp commented Jul 31, 2023

I'm just a contributor, I can't close issues or merge PRs, only collaborators can do it.

The feature is pretty cool, but the idea of implementing it can be a problem. It might turn out that this feature becomes global (at some point there would be some debate about this) instead of just being turned on by a flag, if that happens it would make cheating a bit easier (after committing all the files a cheater can create a .gitignore file with just * written). Some people can persuade people who review their project by just using the “-g” flag by saying something like “it's meant to run globally”.

I like the idea of .*ignore files being unnecessary, it allows us to create a flag called --ignore (to be explicit) that accepts a simple RegEx or wildcards as a filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants