Skip to content

Conversation

joehan
Copy link
Contributor

@joehan joehan commented Jan 3, 2019

Description

Adding useful default .gitignore files for when the function directory is init - see bug # 72516785

Scenarios Tested

Created a new javascript functions project, functions/.gitignore exists
Created a new typescript functions project, functions/.gitignore includes the new additions

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Jan 3, 2019
@joehan joehan requested a review from thechenky January 3, 2019 22:41
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling 8792c1f on jh-add-gitignore into 07f792c on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling 8792c1f on jh-add-gitignore into 07f792c on master.

@coveralls
Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage remained the same at 60.117% when pulling 7e6ab55 on jh-add-gitignore into 7c01343 on master.

# Typescript v1 declaration files
typings/

lib/
Copy link
Contributor

Choose a reason for hiding this comment

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

If I create a file structure:

project/
  functions/
    lib/
      [built files]
    src/
      index.ts
      lib/
        foo.ts

src/lib/foo.ts is ignored because of this rule (and so does lib/, which is intended). just saying lib/ is a little too open (though I get that we want to ignore built files).

That said, there are two options:

  • assuming the lib directory will always contain built js and js.map files (as ignored above), we should remove lib from .gitignore. No files other than those two extensions means that git will ignore the directory
  • if we want to allow lib folders elsewhere (and not cause headaches for folks, probably a good idea), we should include the line !src/**/lib/ which tells it to allow any folder named lib as a sub directory of a folder named src

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, if you use either /lib/ or lib/**/* it will only match from the root (or rather, from the location of the .gitingore file) which is probably what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. Yup. I struggled to get that right. Thank you @jsayol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - I'm going to remove the line, since it doesn't look like the Typescript compiler will put any files other than .js or js.map files into the output directory. If there is something else in there that wouldn't be matched by the first 2 rules, it was probably added by the developer for some reason, and ignoring it by default is probably not the expected behavior. Also, if the user changes the output directory for some reason, the /lib/ rule will be extraneous.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still consider adding /lib/ to the .gitignore for typescript, and that will match only the root level lib, which contains the built files, and not pick up anything else.

Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

I'm approving this, though assigning to @thechenky for final 👍

Also, consider if a line should be added to changelog.txt for this change (ask @thechenky or @bkendall) for a reference about the format if you're unclear)

@bkendall
Copy link
Contributor

bkendall commented Jan 7, 2019

(I also just updated with master, so if you change something locally, git pull first before you commit)

@joehan joehan merged commit 454d501 into master Jan 8, 2019
@joehan joehan deleted the jh-add-gitignore branch January 8, 2019 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Manual indication that this has passed CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants