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

Fix bug where some directories were skipped #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fredden
Copy link

@fredden fredden commented Aug 21, 2023

I noticed this bug while working on another contribution. After calling opendir(), the first two results are thrown away. While these are usually . and .., that's not guaranteed. In my case, the language directory in question was one of these first two entries, so no documentation was generated. The first change here is no longer throwing away the first two directory entries. The second change, is to ignore all entries which have a dot in their name. No language codes include a dot. (Some have a dash/hyphen/underscore, but no dots.) This realisation lead to cleaner code and more efficient run as we no longer have to ask the file-system if something is a directory or not as often.

@fredden

This comment was marked as outdated.

@derickr
Copy link
Member

derickr commented Jul 30, 2024

Is this still relevant?

@fredden
Copy link
Author

fredden commented Jul 31, 2024

Is this still relevant?

Yes, I think it is. The bug still exists. However, it's likely that the system on which this runs for production just happens to return the . and .. entries first (by chance), so the bug isn't witnessed there.

continue;
}
if ($f === '.git') {
if (strpos($f, '.') !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be simply if ($f[0] === '.'), a . could exist in the reset of the filename.

Copy link
Author

Choose a reason for hiding this comment

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

We could do this, however a dot is not a valid character in a language code.

@derickr
Copy link
Member

derickr commented Aug 13, 2024

@fredden ?

@fredden
Copy link
Author

fredden commented Aug 13, 2024

@fredden ?

Hi. I'm away from my laptop currently, so I can't easily update the code here. Would you like me to apply the change you suggested? I think the code is fine as-is, but I will make changes as required.

@fredden fredden requested a review from derickr August 22, 2024 21:53
@jimwins
Copy link
Member

jimwins commented Nov 19, 2024

This is a bit of a tangent, but just to throw it out there -- this script should not live in the systems repo, we should move it into doc-base so developing/maintaining it sits with the team of folks working on the documentation tooling.

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