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

Number of fixes (typos, error names unification) #184

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Conversation

ligurio
Copy link
Member

@ligurio ligurio commented Jul 12, 2021

PLEASE MERGE IT WITH REBASE (without squash)

What has been done? Why? What problem is being solved?

I didn't forget about

  • Tests
  • Changelog
  • Documentation

Closes #???

crud/borders.lua Outdated Show resolved Hide resolved
@ligurio ligurio force-pushed the ligurio/fixes branch 2 times, most recently from b928ecc to 543ce01 Compare July 12, 2021 10:37
@ligurio ligurio requested a review from AnaNek July 12, 2021 10:43
crud/borders.lua Outdated Show resolved Hide resolved
@ligurio ligurio mentioned this pull request Jul 13, 2021
@ligurio ligurio force-pushed the ligurio/fixes branch 9 times, most recently from 142624f to c717aa4 Compare July 15, 2021 15:55
README.md Outdated
@@ -66,7 +66,7 @@ local result, err = crud.insert_object(space_name, object, opts)

where:

* `space_name` (`string`) - name of the space to insert an object
* `space_name` (`string|number`) - name or numerical id of the space to insert an object
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it tested anyhow?

Won't it be a problem because on different replicasets the same space could have different id?

Copy link
Member Author

Choose a reason for hiding this comment

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

No actually, I have reverted these changes.

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

I see no problems here. LGTM.

Should not we include the error naming change into the changelog? It is the user visible thing.

@mRrvz
Copy link
Contributor

mRrvz commented Jul 26, 2021

Please, update the CHANGELOG.

We use `errors.new_class()` that declares error names. Error names
usually has a form "<OperationName>Error", but some error codes has a form
"<OperationName>". This patch makes all error names conform to the first
form.
@ligurio
Copy link
Member Author

ligurio commented Jul 26, 2021

@mRrvz Updated.

@ligurio ligurio merged commit ea7560d into master Jul 27, 2021
@ligurio ligurio deleted the ligurio/fixes branch July 27, 2021 11:56
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.

8 participants