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

Add object files to default ignore patterns #491

Merged
merged 3 commits into from
Sep 20, 2016
Merged

Conversation

malept
Copy link
Member

@malept malept commented Sep 16, 2016

Have you read the section in CONTRIBUTING.md about pull requests?

Yes

Summarize your changes:

Avoids the step of having to ignore .o files when signing the app for OSX/macOS (per the Electron docs). Also ignore Windows .obj files for completeness.

@jlord @kevinsawicki @zeke do you see any potential issues with adding this regex to the default ignore list?

Are your changes appropriately documented?

Yes

Do your changes have sufficient test coverage?

Yes

Does the testsuite pass successfully on your local machine?

Yes

Avoid the step of having to ignore .o files when signing the app for
OSX. Also ignore Windows .obj files.
@kevinsawicki
Copy link
Contributor

do you see any potential issues with adding this regex to the default ignore list?

Looks good to me 👍

Somewhat related, here are the native module build folder ignore patterns Atom uses:

https://github.com/atom/atom/blob/c45b978816c111837753dbc4fe9f762e522a2c24/script/lib/include-path-in-packaged-app.js#L43-L49

@malept malept merged commit 7dc3266 into master Sep 20, 2016
@malept malept deleted the ignore-object-files branch September 20, 2016 00:58
@zeke
Copy link
Contributor

zeke commented Sep 20, 2016

Looks like I'm late to the party, but why does this new regex start with a backslash? All the others start with a slash...

'\\.o(bj)?$'

@malept
Copy link
Member Author

malept commented Sep 20, 2016

The others start with a slash because they indicate a folder name. This indicates a file extension.

@zeke
Copy link
Contributor

zeke commented Sep 20, 2016

got it. thanks.

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