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

Rename `options` decorator #558

Closed
SergioBenitez opened this Issue Dec 30, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@SergioBenitez
Contributor

SergioBenitez commented Dec 30, 2016

Rocket uses options as a decorator to declare an HTTP options route. Diesel uses it to declare options on schemas. This causes a collision when the two libraries are used together, resulting in a compilation error.

It seems that the options decorator isn't used frequently by end-users in Diesel, while options in Rocket is one of the core decorators. Would you consider renaming options to schema_options or table_options, or something else, so that the two can coexist before macro renaming is allowed?

@killercup

This comment has been minimized.

Member

killercup commented Jan 1, 2017

FTR: Since macros 1.1 does not allow supplying parameters to the derive calls itself, we unfortunately need to deal with attribute namespacing. Diesel already does a bit of that with attributes like changset_options and less generic names like table_name, or primary_key, but I can see that options is a problem.

Luckily, as you note, this should not be a user-facing breaking change. The options attribute is only used in the macros infer_schema!, infer_schema_from_table!, and embed_migrations!. I would accept a PR to change these attribute names to something more specific, like infer_schema_options, infer_table_from_schema_options, and embed_migrations _options.

It should suffice to change the names here, here, and here.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 3, 2017

BTW https://github.com/SergioBenitez/Rocket/blob/24805bbf16d25a21d6a926c86035ac905524c71c/scripts/test.sh#L80-L84 I super promise everything is fine now. Macros 1.1 fixed everything.

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