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

Cleaned some code #706

Merged
merged 2 commits into from
Jan 31, 2019
Merged

Cleaned some code #706

merged 2 commits into from
Jan 31, 2019

Conversation

mariecurried
Copy link
Contributor

@mariecurried mariecurried commented Jan 31, 2019

Made some changes to improve the quality of the code and comments:

  • Only generate the defer code to release the lock on the value directory if it is different from the main directory
  • Removed a period from ErrValueThreshold to keep it consistent with the other errors
  • Removed an unused image
  • Make explicit that if the directory or the value directory don't exist, they will try to be created
  • Remove an impossible case from a switch
  • Replace ts += 1 for t++

This change is Reviewable

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 6 files at r1.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @marigonzes)


options.go, line 35 at r1 (raw file):

	// Directory to store the data in. If it doesn't exist, it will
	// try to be created for you.

Rephrase this sentence to something like "If it doesn't exist, Badger will try to create it for you". The current phrasing makes it sound like the directory will create itself.

Same for the sentence below.


txn.go, line 624 at r1 (raw file):

		err := cb.commit()
		cb.user(err)
	case cb.user == nil:

I am not sure if this should be removed but it should be the second case (right below "case cb == nil").

Copy link
Contributor Author

@mariecurried mariecurried left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @martinmr)


options.go, line 35 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
	// Directory to store the data in. If it doesn't exist, it will
	// try to be created for you.

Rephrase this sentence to something like "If it doesn't exist, Badger will try to create it for you". The current phrasing makes it sound like the directory will create itself.

Same for the sentence below.

Done.


txn.go, line 624 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I am not sure if this should be removed but it should be the second case (right below "case cb == nil").

Done. Initially I removed it, because I noticed it was possible for it to be nil in the lines above. Should have just move it in the first place.

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 3 of 6 files reviewed, all discussions resolved

@martinmr martinmr merged commit 28ef9bf into hypermodeinc:master Jan 31, 2019
@martinmr
Copy link
Contributor

Approved and merged. Thanks for your PR.

@manishrjain
Copy link
Contributor

Thanks for your PR. But, why delete the sketch? Wasn't hurting anybody.

@mariecurried
Copy link
Contributor Author

@manishrjain, now I feel bad for the little badger 😢
Can you bring it back?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants