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

chore(common): adds .gitignore to prevent noise from 16.0 changes #6782

Closed
wants to merge 1 commit into from

Conversation

jahorton
Copy link
Contributor

#6525's 🎡 chain made a lot of file system changes, and those are only going to be increasing with the 🎢 PRs. When swapping from master to stable-15.0, without this in place...

image

And that's before all the package-lock.json changes (for Web builds) kick in that need to be manually ignored during development!

Fortunately, one small .gitignore later, and we can mask all the 16.0+ tidbits with relative ease.

@keymanapp-test-bot skip

@jahorton jahorton added this to the A16S4 milestone Jun 16, 2022
@jahorton jahorton requested a review from mcdurdin as a code owner June 16, 2022 02:15
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jun 16, 2022

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@mcdurdin
Copy link
Member

I'd prefer not to add such a big hammer into the .gitignore -- it's too global.

Note that there are ways to ignore files locally if this is troublesome to you. git clean -fd is a wholesome command as well if you are doing extensive maintenance on the stable branch -- this is what I do personally, as a rebuild doesn't take that long anyway after switching back to alpha development, and is not a bad idea.

🙂

@jahorton
Copy link
Contributor Author

I'd prefer not to add such a big hammer into the .gitignore -- it's too global.

a big hammer

too global

😕 How so? We're not exactly going to be adding new features, etc within the common/ folder on stable-15.0, and both lines are for things we'd very much tend to ignore anyway if we were to do so - it's all build-product ignore stuff.

@mcdurdin
Copy link
Member

😕 How so? We're not exactly going to be adding new features, etc within the common/ folder on stable-15.0, and both lines are for things we'd very much tend to ignore anyway if we were to do so - it's all build-product ignore stuff.

Look, I agree this would probably work for stable-15.0 and we'd most likely get away with it without any problems. But I don't feel like it's good development practice. A bit more on why I still don't like it:

  1. It isn't for anything generated by code on this branch: it's a patch for (from this branch's perspective) unrelated, externally generated content. .gitignore should be for stuff related to the current code.
  2. It impacts a bunch of modules -- not just web-based components but also Keyman Core. Probably safe, but not good practice generally. Particularly .inc.ts -- there's no reason why this wouldn't be a valid file extension somewhere -- it just happens to be generated right now.

Now if you wanted to update common/.gitignore to list each build folder, that'd mitigate the hammer effect, and I'll be okay with that. That doesn't fix the 'external' issue above, but it's explicit and targeted.

HTH 😁

@jahorton
Copy link
Contributor Author

😦

@jahorton jahorton closed this Jun 17, 2022
@jahorton jahorton deleted the chore/common/file-migration-.gitignore branch June 17, 2022 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants