Reworked dependencies and table definitions #72
Conversation
…vided default (fixed issue datajoint#41)
…ont classes (e.g. dj.Manual vs dj.Relvar)
+dj/GeneralRelvar.m
Outdated
| s = sprintf(', %s',self.nonKeyFields{:}); | ||
| fprintf('Dependent attributes: %s', s(2:end)) | ||
| hdr = self.header; | ||
| try |
There was a problem hiding this comment.
Why use try...catch instead of explicit check for presence of tableHeader attribute?
There was a problem hiding this comment.
Perhaps you don't mind addressing this question?
+dj/GeneralRelvar.m
Outdated
| end | ||
| maxRows = 12; |
There was a problem hiding this comment.
Like in Python, maxRows ought to be configurable via dj.set.
| @@ -81,7 +81,7 @@ | |||
| attrs.isNumeric = ~cellfun(@(x) isempty(regexp(sprintf('%s',x), ... | |||
| '^((tiny|small|medium|big)?int|decimal|double|float)', 'once')), attrs.type); | |||
| attrs.isString = ~cellfun(@(x) isempty(regexp(sprintf('%s',x), ... | |||
| '^((var)?char|enum|date|year|time|timestamp)','once')), attrs.type); | |||
| '^((var)?char|enum|date|(var)?binary|year|time|timestamp)','once')), attrs.type); | |||
There was a problem hiding this comment.
Why is (var)? match labeling repeated here?
There was a problem hiding this comment.
It is not repeated. The first (var)? belong to varchar
There was a problem hiding this comment.
Ah, ok I've mistaken it for the match labeling syntax.
There was a problem hiding this comment.
it's not labeling but an optional character sequence in regular expressions. (var)?char is equivalent to varchar|char
There was a problem hiding this comment.
Yeah I know - I mistook it for the (?<groupname>) syntax.
| @@ -32,6 +32,7 @@ | |||
| % computed: tableName with '__' | |||
| allowedTiers = {'lookup' 'manual' 'imported' 'computed' 'job'} | |||
| tierPrefixes = {'#', '', '_', '__', '~'} | |||
| tierClasses = {'dj.Lookup' 'dj.Manual' 'dj.Imported' 'dj.Computed' 'dj.Jobs'} | |||
There was a problem hiding this comment.
I much prefer that we place the tierPrefix information directly inside the corresponding class, rather than using the class as mere naming magic.
There was a problem hiding this comment.
That can be implemented once we completely transition to the new implementation. The old implementation did not have corresponding classes and it is still being used.
There was a problem hiding this comment.
I understand but we could go ahead and push the tier prefix information into the class while still supporting the older syntax - I don't like how the UserRelation classes are completely empty and placing tier prefix can be a useful information in the class.
There was a problem hiding this comment.
I agree but I think we can hold off with that since it modifies existing functions. I am working on unit regression tests and changes like these will be easier to make.
There was a problem hiding this comment.
Ok I agree that such changes are better after unit tests are in place.
| @@ -7,7 +7,7 @@ | |||
| properties(SetAccess = private) | |||
| package % the package (directory starting with a +) that stores schema classes, must be on path | |||
| dbname % database (schema) name | |||
| prefix='' % optional table prefix, allowing multiple schemas per database | |||
| prefix='' % optional table prefix, allowing multiple schemas per database -- remove this feature if not used | |||
There was a problem hiding this comment.
Let's talk about this feature - I have never seen it being used and it may be an unnecessary complexity.
There was a problem hiding this comment.
yes, we need to remove it. Only Florian has used it when they had a very unreasonable constraint: they were now allowed to create more databases on their server. I am not even sure if it works since no one here has used it.
+dj/Table.m
Outdated
| end | ||
|
|
||
|
|
||
| function str = re(self) | ||
| % dj.Table/re - "reverse engineer" the table declaration. | ||
| % alias for self.re | ||
| warning 'dj.Table/re is deprecated: Use dj.Table/describe instead' |
There was a problem hiding this comment.
Let's include message as to when we plan on fully removing this deprecated feature.
There was a problem hiding this comment.
Is there a good reason to remove it in the near future? I think we will let it linger around for a bit longer and then add the message once no one uses it anymore.
There was a problem hiding this comment.
If it says deprecated, I think it only makes sense that we remove it at some point. Not sure how immediate it should be though.
There was a problem hiding this comment.
I have updated this, although the word deprecated already implies that the feature may no longer be supported in the future.
+dj/Table.m
Outdated
| @@ -493,8 +466,8 @@ function syncDef(self) | |||
| fprintf('File %s.m is not found\n', self.className); | |||
| else | |||
| if ~dj.set('suppressPrompt') ... | |||
| && ~strcmpi('yes', dj.ask(sprintf('Update table declaration in %s?',path))) | |||
| disp 'No? Table declaration left untouched.' | |||
| && ~strcmpi('yes', dj.ask(sprintf('Update table definition in %s?',path))) | |||
There was a problem hiding this comment.
Include explicit message stating that the table will be subclassed from the correct UserRelation - this will help point out the new syntax.
…y method for projection.
dj.Manual,dj.Lookup,dj.Imported,dj.Computed, anddj.Part.dj.newanddj.Relvar/describeto use the new format.