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

Ignore some hidden files and folders #131

Merged
merged 1 commit into from
Jan 18, 2019
Merged

Conversation

mzfr
Copy link
Contributor

@mzfr mzfr commented Oct 15, 2018

Fixes #123

@Rechi
Copy link
Member

Rechi commented Oct 15, 2018

Better would be to not exclude all .* files/folders. Just exclude .git explecitly.

@mzfr
Copy link
Contributor Author

mzfr commented Oct 15, 2018

But why do we want to process any folder that is hidden?

@Rechi
Copy link
Member

Rechi commented Oct 15, 2018

Because those should be also checked if they contain none allowed files.

@razzeee
Copy link
Member

razzeee commented Oct 15, 2018 via email

@Rechi
Copy link
Member

Rechi commented Oct 15, 2018

Repo vs single addon seperation is fine for me.

Edit: Is PR check implemented as repo or single addon check?

@mzfr
Copy link
Contributor Author

mzfr commented Nov 2, 2018

@razzeee is there any changes required in this?

@razzeee
Copy link
Member

razzeee commented Nov 2, 2018

@Rechi the PR flag get's handed down in both cases and is taken care of in a single check it executes at the moment.

I'm torn now, while I think me design above is correct, it would lead to you running the reporter local/in your git branch and not getting errors. Then sending it to our git and probably getting errors :/

@Rechi
Copy link
Member

Rechi commented Nov 13, 2018

@razzeee what do you think about ignoring files which are marked as export-ignored in git (https://git-scm.com/docs/gitattributes#_creating_an_archive)?

@razzeee
Copy link
Member

razzeee commented Nov 13, 2018

Can you explain how that would help?

@Rechi
Copy link
Member

Rechi commented Nov 13, 2018

It wouldn't ignore all files/folders starting with . by default, but you could exactly define which files/folders to ignore (eg also tests).

@razzeee
Copy link
Member

razzeee commented Nov 13, 2018

I hoped to keep it convention over configuration :/

@Rechi
Copy link
Member

Rechi commented Nov 13, 2018

As long as the check run at xbmc/repo-* doesn't ignore any files, I would be okay with ignoring .* files.
But as you already have mentioned that will cause different results.

@razzeee
Copy link
Member

razzeee commented Nov 13, 2018

I think we need two modes. Additional to PR.

  • Ignores those files, only used local and in developers repo
  • Ignores nothing for our xbmc/repo-*

While it's not fun to get new errors when you open the PR version, it's better then having no error reporting at all. If we don't implement it, nobody will use it for their local/dev repos, as they will get false positives all the time.

@@ -39,7 +39,8 @@ def create_file_index(path: str):
file_index = []
for dirs in os.walk(path):
for file_name in dirs[2]:
file_index.append({"path": dirs[0], "name": file_name})
if not file_name.startswith("."):

This comment was marked as spam.

file_index.append({"path": dirs[0], "name": file_name})
for root, _, files in os.walk(path):
for file_name in files:
if not file_name.startswith(".") and not os.path.basename(root).startswith("."):

This comment was marked as spam.

@mzfr mzfr force-pushed the hidden-files branch 2 times, most recently from 07f3a3f to 98c4095 Compare November 20, 2018 11:33
@@ -49,7 +49,9 @@ def create_file_index(path: str):
if ".git" in folders:
folders.remove(".git")
for file_name in files:
file_index.append({"path": root, "name": file_name})
if not os.path.basename(root).startswith("."):

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@mzfr
Copy link
Contributor Author

mzfr commented Dec 10, 2018

Now, this is what this code is doing:

Say we have the following directory structure:

├── .directory
├── folder1
│   ├── file.py
│   └── .sub_hidden1
└── .hidden
    ├── folder2
    │   ├── .directory
    │   ├── file2.py
    │   └── .sub_hidden2
    └── hidden_file.py

Now if we run this code on the above-mentioned directory we'll get the following file_index

[{'path': '/home/mzfr/dev/Test_folder/folder1', 'name': 'file.py'}, {'path': '/home/mzfr/dev/Test_folder/folder1', 'name': '.sub_hidden1'}, {'path': '/home/mzfr/dev/Test_folder/.hidden/folder2', 'name': '.sub_hidden2'}, {'path': '/home/mzfr/dev/Test_folder/.hidden/folder2', 'name': 'file2.py'}]

I think this is what we want.

@Rechi
Copy link
Member

Rechi commented Dec 10, 2018

The current PR title tells that everything in .hidden directory is ignored too, but the code doesn't ignore it.

@@ -46,10 +46,14 @@ def create_file_index(path: str):
"""
file_index = []
for root, folders, files in os.walk(path, topdown=True):
if ".directory" in files:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only skip directories named .directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to say we should ignore all the hidden files in the root directory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is special about a folder named .directory that it should be ignored, but e.g. not a folder named .abc?
The current code does ignore .directory folders everywhere, not only in the root directory.
The current PR title is "Ignore hidden files and folders", which means ignore all files and folder for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.directory is a file, that's why I am checking it in files as

if ".directory" in files:

Also why I am ignoring it? Because I thought since it doesn't have any information usable by us so it would be good to ignore it.

Yes, I named it when I thought we were going to ignore everything. I'll change the name accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@razzeee @mzfr I still don't get the reason to exclude a file named .directory. What's this special file that it shouldn't be listed in the file index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is basically used by file managers to keep track of the direcotries and also used for the preview of the direcotries.

Why I decided to ignore them? Since these files might be present in the folders(root) of addon but they don't consist any data that can be used by an addon so I simply decided to ignore them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's like .DS_Store and Thumbs.db on other OSes?

Still the checker shouldn't ignore the file, as the checker now doesn't warn about those files submitted in an add-on submission.

@mzfr mzfr changed the title Ignore hidden files and folders Ignore some hidden files and folders Dec 10, 2018
@mzfr
Copy link
Contributor Author

mzfr commented Dec 31, 2018

@razzeee Is there anything other changes required in this?

@razzeee razzeee merged commit 76377d9 into xbmc:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants