Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Fix inheritance #199

Merged
merged 25 commits into from
Dec 28, 2015
Merged

Fix inheritance #199

merged 25 commits into from
Dec 28, 2015

Conversation

klokane
Copy link
Contributor

@klokane klokane commented Dec 16, 2015

Reworked inheritance:

  • fix correct order in extend element for inheritance from older ancestor to newest
  • allow inheritance for primitive members
  • fixed inheritance for RenderJSONVisitor
  • introduce ElementMerger to handle correctly/standardized way inheritance resolve
  • refactoring ExpandVisitor
  • move members out of IElement (it now provide just Interface and some helpers)
  • little refactoring in TypeQueryVisitor (expose enum ast typedef)
  • correct order "data structures" according to membership/inheritance (most dependent objects are registered latest)

To be resolved:

  • cripled some schema test after rebase (done)
  • add test for primitive types inheritance (done)
  • add test for complex membership/inheritance
    (test are created, just needs to added to repository and check if schema is generated correctly)

Resolved issues:

@@ -6,7 +6,6 @@
// Copyright (c) 2015 Apiary Inc. All rights reserved.
//

#include <iterator>
Copy link
Member

Choose a reason for hiding this comment

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

Curious, any particular reason you've moved this down? In most languages it preferred to put system includes before local includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In short? In C/C++ is preferred place system includes below specific :)

In long:

// file a.h
std::string fn();
// file a.cc
#include <string>
#include "a.h"

std::string fn() { return "hello"; }

a.cc will be compiled ok

but

// file: b.cc
#include "a.h"

// something...

b.cc compilation will fail with something like unknown type 'std::string'

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's a good point. It's no longer been a problem in Objective-C with the introduction of clang modules. I completely forgot it's not standard/default across all compilers.

👍

@klokane
Copy link
Contributor Author

klokane commented Dec 17, 2015

Last commit (103c0bf) is for discussion, it fix problems with schema fixtures, but fixtures definition in schema are not correct. There should be selected one value for enum instead of array of possible values.

Correct implementation is commented out by #if 0 block

There still missing one schema to fix.

@klokane
Copy link
Contributor Author

klokane commented Dec 17, 2015

Hmm interesting, sent patch (commented out part) fixes #176 :)

Jiri Kratochvil added 3 commits December 17, 2015 14:04
@klokane klokane force-pushed the klokane/fix-inheritance branch 2 times, most recently from d71ce17 to 23b49f1 Compare December 18, 2015 13:52
@klokane
Copy link
Contributor Author

klokane commented Dec 19, 2015

Curent status

(member c8 - key should be expanded as number - because it is variable)

@klokane
Copy link
Contributor Author

klokane commented Dec 23, 2015

@w-vi review addressed. There are no tests :( in current tests implementation it is not possible to prepare test with comparable output.

We need implement querying for refract to build tests for similar cases

@w-vi
Copy link
Contributor

w-vi commented Dec 23, 2015

My part is done, I think it's ready for merge ... so leaving it to @pksunkara if he has anything to comment on

@klokane
Copy link
Contributor Author

klokane commented Dec 23, 2015

BTW why "continuous-integration/appveyor" CI is (again and again) yellow? I mean, we should remove it form CI proces it it does not work.

@pksunkara
Copy link
Contributor

We should be relying on the other 2 green things, not the continuous-integration/appveyor which is just an aggregator.

 continuous-integration/appveyor/branch
 continuous-integration/appveyor/pr

The yellow one is an issue but hopefully appveyor will fix it soon. We can try to forget about it until then since we need the other CI statuses.

@klokane
Copy link
Contributor Author

klokane commented Dec 23, 2015

@pksunkara yes, I know about it. But if I remember it is not working for few months. It is confusing

@pksunkara
Copy link
Contributor

yes, I know about it. But if I remember it is not working for few months. It is confusing

I know and I feel the same. 😭 But we really do need the windows CI. 😞


# Date2 (Date1)
## Sample
2012-12-01
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to have 2012-12-02 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, yes. But it is not important, so it can stay as is.
In final inheritance is used sample from Date3 so test is able to check if it is inherited in required way

@pksunkara
Copy link
Contributor

@klokane Finished review. Just minor stuff to fix.

Jiri Kratochvil added 2 commits December 28, 2015 15:24
- unknown members are now copied into original element
- add set of keywords to be ignored while merge
@klokane
Copy link
Contributor Author

klokane commented Dec 28, 2015

Review addressed, fixedup on to previous commit Fix typo && push --force

pksunkara added a commit that referenced this pull request Dec 28, 2015
@pksunkara pksunkara merged commit 01cf40a into master Dec 28, 2015
@pksunkara pksunkara deleted the klokane/fix-inheritance branch December 28, 2015 14:36
grncdr added a commit to contentful/slash-developers that referenced this pull request Jan 5, 2016
This fixes the build until apiaryio/dredd#319 and apiaryio/drafter#199 are resolved, at which point we can make upgrade and make the changes outlined in https://github.com/contentful/slash-developers/issues/149
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants