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.

Sign up for GitHub

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

Jump to bottom

Refactor/extract builtin contracts #199

Merged
merged 2 commits into from
Sep 15, 2015
Merged

Refactor/extract builtin contracts #199

merged 2 commits into from
Sep 15, 2015

Conversation

PikachuEXE
Copy link
Collaborator

This is for a comment in another PR

Changes

  • Create module Builtin as suggested
  • Change all references in code
  • Change all references in doc
  • Auto include Builtin when module Contracts included
    (but not when extended, since I am not sure)

Missing stuff

  • Test for built-in contract usage without namespacing when Contracts included

@alex-fedorov
Copy link
Collaborator

You can include Builtin into Contracts directly at a module level. Then you
don't need to call include Builtin inside of included.

This way you can still refer them as Contracts::Maybe anywhere.

Then I would add to tutorial example what happens if including
Contracts::Builtin directly
On Sep 14, 2015 4:08 AM, "PikachuEXE" notifications@github.com wrote:

This is for a comment in another PR
#198 (comment)
Changes

  • Create module Builtin as suggested
  • Change all references in code
  • Change all references in doc
  • Auto include Builtin when module Contracts included (but not when
    extended, since I am not sure)

Missing stuff

  • Test for built-in contract usage without namespacing when Contracts
    included

You can view, comment on, or merge this pull request online at:

#199
Commit Summary

  • * Move Built in contract classes into namespace Builtin
  • ~ Change doc for changed namespace
  • * Ensure when module Contracts is included, Builtin module is
    also included

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#199.

@PikachuEXE
Copy link
Collaborator Author

So the desired objectives are the following?:

  1. Be able to use built in contracts without namespace after Contracts included
  2. Be able to include Contracts::Core & Contracts::Builtin, and still works like (1)
  3. Be to reference built in contracts as something like Contracts::Maybe

@alex-fedorov
Copy link
Collaborator

@PikachuEXE Exactly!

@PikachuEXE PikachuEXE force-pushed the refactor/extract-builtin-contracts branch from 6912420 to c32cc41 Compare September 14, 2015 09:33
@PikachuEXE
Copy link
Collaborator Author

Updated & squashed

In the next major bump (breaking),
Is that only Contracts::Builtin::Maybe is usable (or preferred)?

@waterlink
Copy link
Collaborator

No, we want to always have Builtin included in Contracts, so Contracts::Maybe is usable and preferred.

Extraction to Builtin is only done to make it possible to include Contracts::Builtin along with Contracts::Core when your code base don't have name clashes with built-in contracts.

Just include Contracts is a no go, because we define much more stuff than just built-in contracts, like: Engine and Support - which are pretty likely to clash.

waterlink
waterlink reviewed Sep 14, 2015
View reviewed changes
TUTORIAL.md
@@ -17,7 +17,7 @@ contracts.ruby brings code contracts to the Ruby language. Code contracts allow
A simple example:

```ruby
Contract Contracts::Num, Contracts::Num => Contracts::Num
Contract Contracts::Builtin::Num, Contracts::Builtin::Num => Contracts::Builtin::Num
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, we don't want to expose this information to users. We want them to still either:

  • include Contracts::Core and use Contracts::Maybe (or C::Maybe)
  • include Contracts::Core, Contracts::Builtin and use Maybe

So in TUTORIAL.md good changes are only for error messages and validator list

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I think about validator list, it does not need to have Builtin:: in it too.

@waterlink
Copy link
Collaborator

@PikachuEXE Looks very good so far. Left some comments around Builtin:: usage in docs and code.

Ideally, since include Builtin have been added in Contracts, we don't need to use Builtin:: anywhere, nor our users need to know about Contracts::Builtin::*, they need to know only about include Contracts::Core and include Contracts::Builtin and about Contracts::ContractName and ContractName shortcut (in case Contracts::Builtin is included).

@PikachuEXE PikachuEXE force-pushed the refactor/extract-builtin-contracts branch from c32cc41 to cd8ea54 Compare September 15, 2015 02:00
@PikachuEXE
Copy link
Collaborator Author

Most changes reverted except a few places
See my line note(s)

@waterlink
Copy link
Collaborator

LGTM

waterlink added a commit that referenced this pull request Sep 15, 2015
@waterlink
Merge pull request #199 from PikachuEXE/refactor/extract-builtin-cont…
83507ae 
…racts

Refactor/extract builtin contracts
@waterlink waterlink merged commit 83507ae into egonSchiele:master Sep 15, 2015
@PikachuEXE PikachuEXE deleted the refactor/extract-builtin-contracts branch September 15, 2015 02:18
@waterlink
Copy link
Collaborator

@PikachuEXE Thanks!

@PikachuEXE
Copy link
Collaborator Author

@waterlink Do you know when will this & #198 be released?

@waterlink
Copy link
Collaborator

I will create a PR later today with changelog update and version bump. Then it is up to @egonSchiele to merge & release it to rubygems.

BTW, there is still need to add info on include Contracts::Builtin to TUTORIAL.md in Built-in contracts section.

@waterlink
Copy link
Collaborator

I will add this info later, no need to bother :)

@waterlink
Copy link
Collaborator

created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PikachuEXE @alex-fedorov @waterlink