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

Dev #2

Closed
wants to merge 4 commits into from
Closed

Dev #2

wants to merge 4 commits into from

Conversation

sahilappdeft
Copy link
Collaborator

No description provided.

@Synchro
Copy link
Collaborator

Synchro commented Nov 7, 2024

I see that this PR commits some credentials for both gmail and postgres; that's not good. We don't want anything like that in the commit history, even if you remove it later. Please can you rebase and force-push to remove those commits.

Generally speaking, I do not expect a configuration file like local.py to be in the repo at all, for exactly this reason. The usual thing would be to include a local-dist.py config template that contains empty/harmless placeholders for all the config settings, and a .gitignore entry to prevent adding the real config file to the repo.

@sahilappdeft
Copy link
Collaborator Author

I see that this PR commits some credentials for both gmail and postgres; that's not good. We don't want anything like that in the commit history, even if you remove it later. Please can you rebase and force-push to remove those commits.

Generally speaking, I do not expect a configuration file like local.py to be in the repo at all, for exactly this reason. The usual thing would be to include a local-dist.py config template that contains empty/harmless placeholders for all the config settings, and a .gitignore entry to prevent adding the real config file to the repo.

I have rebased the commits and pushed the code. Please take a look at it.

@Synchro
Copy link
Collaborator

Synchro commented Nov 7, 2024

I'm not sure what you did here, but it's made a big mess. It looks like you rebased from the beginning of the project, not just the changes in this PR.

@Synchro
Copy link
Collaborator

Synchro commented Nov 7, 2024

One other thing – ignore my comment about not including local.py, that's fine, but what it should do is reference env vars set in .env, not to hard-code them. This is done in other places in local.py too.

@sahilappdeft
Copy link
Collaborator Author

One other thing – ignore my comment about not including local.py, that's fine, but what it should do is reference env vars set in .env, not to hard-code them. This is done in other places in local.py too.

Okay, I apologize for this oversight. I’ll make sure to handle it correctly going forward.

@Synchro
Copy link
Collaborator

Synchro commented Nov 8, 2024

It might be best if you scrapped this PR and recreated it with just the changes needed for the delete function and the design tweaks – it's impossible to pick out your changes from what's in here.

@sahilappdeft
Copy link
Collaborator Author

It might be best if you scrapped this PR and recreated it with just the changes needed for the delete function and the design tweaks – it's impossible to pick out your changes from what's in here.

i recreated the PR please look at it.

@Synchro
Copy link
Collaborator

Synchro commented Nov 8, 2024

I don't see a difference - the PR still contains the entire application in this commit.

This reverts commit 174aadc.
@sahilappdeft
Copy link
Collaborator Author

drop

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