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

added ignores for OS, IDE, and Python that were missing. (#446) #447

Closed
wants to merge 0 commits into from

Conversation

agiletechnologist
Copy link
Contributor

Please describe the purpose of this pull request.
This is a fix for #446

How to test
This is for ignoring git commits for Python, OS, and IDE's

Have you tested this PR?
Yes. There are some files to be removed but will not be ignored until removed. Will discuss the removal.

Is your PR over 500 lines of code?
Yes but one file. makes more sense as a single pull.

Copy link
Collaborator

@cpacker cpacker left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -1,6 +1,6 @@
import os

MEMGPT_DIR = os.path.join(os.path.expanduser("~"), ".memgpt")
MEMGPT_DIR = os.getenv(os.path.join("MEMGPT_CONFIG_PATH",".memgpt"), default=os.path.join(os.path.expanduser("~"), ".memgpt"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think MEMGPT_CONFIG_PATH is currently the actual path of the config? so ~/.memgpt/config, rather than the directory.

So this should maybe be:

MEMGPT_DIR = os.getenv("MEMGPT_CONFIG_PATH", default=os.path.join(os.path.expanduser("~"), ".memgpt"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MEMGPT_DIR was not supposed to be included only the one for .gitignore.

@agiletechnologist
Copy link
Contributor Author

agiletechnologist commented Nov 16, 2023 via email

@sarahwooders
Copy link
Collaborator

@agiletechnologist could you remove the constants.py diff? Then we can merge this.

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