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

Simple Clean up, .gitignore improved, PR template added #227

Merged
merged 15 commits into from
Nov 20, 2023

Conversation

smttsp
Copy link
Contributor

@smttsp smttsp commented Oct 5, 2023

Summary

What is the reason for this Pull-Request?

This PR improves the .gitignore as well as adds a PR template to be filled in any PR created.

Added the following sections:

  • Byte-compiled / optimized / DLL files
  • development environment configuration files (.idea, .vscode)
  • Python Virtual Environments (common ones: venv, env, .direnv etc)
  • C/c++ extensions (.so)
  • Distribution / packaging (.egg, lib, lib64, wheels etc)
  • Installer logs
  • Unit test / coverage reports
  • log files
  • IPython (e.g. profile_default/)
  • mkdocs documentation
  • mypy caches
  • compressed files (zip, gz, tar, 7z etc)
  • other files (output, wandb)

Impact

What is the scope of your changes and who/where do you think they will affect?

  • improved gitignore
  • new template

Testing

What testing have you done and verified? If you can't test, please say why that is.

  • I have tested both locally
  • I am using the template now

Ticket(s) or Conversations

What Git or Slack items (Issues, threads etc) are specifically related to this work? Please link them here.

-N/A

References

Do you have any relevant URLs or Wiki pages that could help give background information for the changes in this Pull-Request? These are usually StackOverflow posts, Notion wiki pages, Wikipedia articles, or links to APIs and Documentation of third-party packages.

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor Author

@smttsp smttsp left a comment

Choose a reason for hiding this comment

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

ready for review

@smttsp smttsp marked this pull request as ready for review October 5, 2023 03:20
@smttsp smttsp requested a review from jwmueller October 5, 2023 04:00
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9de8f33) 96.04% compared to head (37b0bd8) 95.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
- Coverage   96.04%   95.84%   -0.21%     
==========================================
  Files          16       16              
  Lines         986      986              
  Branches      194      194              
==========================================
- Hits          947      945       -2     
- Misses         20       21       +1     
- Partials       19       20       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jwmueller jwmueller requested a review from sanjanag October 5, 2023 04:45
finalized the gitignore
@jwmueller
Copy link
Member

@smttsp once you've finalized .github/pull_request_template.md

it would be awesome if you can also make a PR with the same template to cleanlab/cleanlab repo!

Copy link
Member

@jwmueller jwmueller left a comment

Choose a reason for hiding this comment

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

There was one overlooked suggestion, which you can commit, and then this LGTM.

Leaving final review + merge to @sanjanag who has relied on the .gitignore more than any other developer of this repo thus far.

@smttsp
Copy link
Contributor Author

smttsp commented Oct 6, 2023

it would be awesome if you can also make a PR with the same template to cleanlab/cleanlab repo!

@jwmueller, would you like me to update gitignore as well?

Copy link
Member

@sanjanag sanjanag left a comment

Choose a reason for hiding this comment

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

Overall, please remove all unnecessary file escapes from the gitignore, it's hard to comment at each one of them.

.gitignore Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@smttsp
Copy link
Contributor Author

smttsp commented Oct 11, 2023

@sanjanag if there is anything in gitignore you wanna keep, I can keep them. Otherwise, I am dropping all the .gitignore related commits. I am not clear on what is needed, and what is not needed in this repo but most of these are pretty common stuff to ignore.

@sanjanag
Copy link
Member

Hi @smttsp ! Your PR template looks good, We don't want to clutter the gitignore with supposedly "common stuff" that does not concern this repo. I'd recommend you only keep the following sections in the gitignore and remove the rest. Thank you.

# development environment configuration files
# Python Environments
# Distribution / packaging
# Data files

@smttsp
Copy link
Contributor Author

smttsp commented Oct 12, 2023

@sanjanag would you like me to have the same exact changes as this one: https://github.com/cleanlab/cleanlab/pull/867/files, so it would be more consistent with the other repo?

.gitignore Outdated
build/
.installed.cfg
*.egg
MANIFEST
Copy link
Member

Choose a reason for hiding this comment

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

Does this line exclude MANIFEST.in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesnt exclude

Copy link
Member

Choose a reason for hiding this comment

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

can you still remove it? I think it's confusing since we have a MANIFEST.in file

.gitignore Outdated
*.cover
.hypothesis/
.pytest_cache/
.coverage*
Copy link
Member

Choose a reason for hiding this comment

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

Does this line exclude .coveragerc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it excludes

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to exclude it, can you please remove it?

.gitignore Outdated

# Misc
results/
image_files*

# datasets
cifar*

cleanlab/*.ipynb
Copy link
Member

Choose a reason for hiding this comment

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

this is not valid in this repo, please remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

.gitignore Outdated

# Misc
results/
image_files*

# datasets
cifar*

cleanlab/*.ipynb
tests/fasttext_data/
Copy link
Member

Choose a reason for hiding this comment

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

this is not valid in this repo, please remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

.gitignore Outdated
*$py.class

# C extensions
*.so
Copy link
Member

Choose a reason for hiding this comment

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

this is not valid in this repo, please remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

many of these are very common stuff in gitignore, in fact, most of these lines are coming from Github's default Python gitignore.

I found being proactive much more effective than being retroactive. We will add common things now, but we won't need to add them to gitignore if/when it happens.

Note that prior to this PR, when I set up my environment, I was seeing 30K files to be committed as the ENV (default) directory wasn't in gitignore.

.gitignore Outdated
*.so


# PyInstaller
Copy link
Member

Choose a reason for hiding this comment

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

this is not valid in this repo, please remove this

.gitignore Outdated
# mypy
.mypy_cache/

# Downloaded datasets for docs
Copy link
Member

Choose a reason for hiding this comment

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

This section is not valid in this repo, please remove this

.gitignore Outdated
.spyderproject
.spyproject

# Rope project settings
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
venv.bak/
.direnv

# Spyder project settings
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
.python-version

# celery beat schedule file
celerybeat-schedule
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
# celery beat schedule file
celerybeat-schedule

# SageMath parsed files
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
.ropeproject

# mkdocs documentation
/site
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
/docs/source/tutorials/datalab/datalab-files/

# IPython
profile_default/
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
.ipynb_checkpoints

# pyenv
.python-version
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
cleanlab-docs

# PyBuilder
target/
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
# Sphinx documentation
build
_build
cleanlab-docs
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
db.sqlite3
logs/

# Flask stuff:
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
*.mo
*.pot

# Django stuff:
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
pip-log.txt
pip-delete-this-directory.txt

# Translations
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

.gitignore Outdated
*.manifest
*.spec

# Installer logs
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid in this repo, please remove this

@sanjanag
Copy link
Member

Hi @smttsp ! I have added a few more comments, could you please address them? Thanks.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@sanjanag sanjanag merged commit f653207 into cleanlab:main Nov 20, 2023
21 of 22 checks passed
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.

4 participants