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

Disabled the functionality of remove() #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AbhishekSrivastava-23
Copy link

@AbhishekSrivastava-23 AbhishekSrivastava-23 commented Sep 28, 2023

We disable the remove() function by removing the appropriate code, alongside removing its tests and mentions in the README.
This PR closes #2

@benjaminasmall benjaminasmall requested review from dmikhalin, james-whiteside and sam-butcher and removed request for dmikhalin September 28, 2023 09:00
Copy link
Member

@sam-butcher sam-butcher left a comment

Choose a reason for hiding this comment

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

I notice you're merging in from the master branch on your fork - if you're going to be doing multiple pieces of work, it's generally a good idea to make a feature branch on your fork and merge from that. That way, you can more easily work on multiple things at once.

Otherwise, looks good to me.

@sam-butcher
Copy link
Member

sam-butcher commented Sep 29, 2023

Also, when opening a PR that's designed to fix an issue, you should link the issue that you're fixing in the PR description. The PR description should also be more informative than your current one is. Overall, we'd be looking for something more along the lines of:

We disable the remove() function by removing the appropriate code, alongside removing its tests and mentions in the README.
This PR closes #2

This description specifies what you're doing (disabling remove()) as well as how you're doing it (by removing the code). It also mentions the issue you're addressing directly. There's no need to ask for review in the PR description, the need for review is assumed.

For further inspiration, you could consider looking at some PRs on the main TypeDB repository, and/or at the PR template from that repository.

@AbhishekSrivastava-23
Copy link
Author

OK Thanks a lot for your advice.
I'll surely keep this in mind.

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.

Disable constraint removal functionality
2 participants