Skip to content

Commit

Permalink
Apply suggestions from 3rd code review round
Browse files Browse the repository at this point in the history 
Signed-off-by: Konstantin Slavnov <[email protected]>
  • Loading branch information
@zurk
zurk committed May 22, 2019
1 parent 5516ae3 commit d8793b4
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 37 deletions.
63 changes: 35 additions & 28 deletions lookout/style/format/annotations/annotated_data.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,40 +156,42 @@ def get(self, annotation_type: Type[Annotation], span: Optional[Tuple[int, int]]
return self._type_to_annotations[annotation_type][span]

def iter_by_type_nested(self, annotation_type: Type[Annotation],
*additional: Type[Annotation],
*covered_by: Type[Annotation],
start_offset: Optional[int] = None) -> Iterator[AnnotationsSpan]:
"""
Iterate through annotations of the specified type.
Iterate over annotations of the specified type which are covered by some other annotation \
types.
Iteration goes over `annotation_type` objects. Annotations which are specified in
`additional` list added to resulting `AnnotationsSpan` object. Spans of additional
annotations should fully cover spans of `annotation_type`. For example, suppose that you
have *line* and *token* annotations. Each line contains several tokens. If you try to
iterate through *line* and set *token* as `additional` annotation you get only *line*
`covered_by` are added to the resulting `AnnotationsSpan` object. Spans of the additional
annotations should fully cover the spans of `annotation_type`. For example, suppose that
you have *line* and *token* annotations. Each line contains several tokens. If you try to
iterate through *line* and set *token* as `covered_by` annotation you get only *line*
annotation inside `AnnotationsSpan`. It happens because you can annotate *token* with
*line* but not *line* with *token*: *token* is covered by only one line and not vice versa.
So,
manager.iter_annotations(line, token) # gives you only line annotation as output,
# *token* annotations not found
manager.iter_annotations(token, line) # gives you *token* and *line* annotations because
# It is possible to find only one covering *line*
# annotation.
manager.iter_by_type_nested(line, token) # gives you only line annotation as output,
# *token* annotations not found
manager.iter_by_type_nested(token, line) # gives you *token* and *line* annotations
# because it is possible to find only one
# covering *line* annotation.
`additional` can't be empty. If you need to iterate through a single annotation, you should
call `AnnotationManager.iter_annotation()` instead.
`covered_by` can't be empty. If you need to iterate over a single annotation type, you
should call `AnnotationManager.iter_annotation()` instead.
:param annotation_type: Type of annotation to iterate through.
:param additional: Additional annotations that should be added to the main one.
:param covered_by: Additional annotations that should be added to the main one if they \
cover its span.
:param start_offset: Start to iterate from a specific offset. \
Can be used as a key argument only.
:return: Iterator over annotations of the requested type.
"""
if not additional:
raise ValueError("At least one additional annotation must be specified. "
"If you need to iterate through a single annotation use "
if not covered_by:
raise ValueError("At least one covered_by annotation must be specified. "
"If you need to iterate over a single annotation type use "
"`iter_annotation()`.")
types = set(additional) | {annotation_type}
types = set(covered_by) | {annotation_type}
for annotation in self.iter_by_type(annotation_type, start_offset=start_offset):
# Annotations with the same span
same_span_annotations = self._span_to_annotations[annotation.span]
Expand All @@ -209,7 +211,7 @@ def iter_by_type_nested(self, annotation_type: Type[Annotation],
def iter_by_type(self, annotation_type: Type[Annotation], *,
start_offset: Optional[int] = None) -> Iterator[Annotation]:
"""
Iterate through a specific type of annotation.
Iterate over annotations of the specified type.
If you need to iterate through several annotations use \
`AnnotationManager.iter_annotations()` instead.
Expand Down Expand Up @@ -269,11 +271,16 @@ def find_covering_annotation(self, annotation_type: Type[Annotation],
:raise NoAnnotation: There is no such annotation that overlaps with the given interval.
:return: `Annotation` of the requested type.
"""
return self._find_annotations(
annotation_type, start, stop, action="contains",
inspect=lambda span_start, span_stop: self._check_spans_overlap(
start, stop, span_start, span_stop) if start == stop else
span_start <= start and stop <= span_stop)
if start == stop:
def check_cover(span_start, span_stop):
return self._check_spans_overlap(start, stop, span_start, span_stop)

else:
def check_cover(span_start, span_stop):
return span_start <= start and stop <= span_stop

return self._find_annotations(annotation_type, start, stop, action="contains",
inspect=check_cover)

@classmethod
def _check_spans_overlap(cls, start1: int, stop1: int, start2: int, stop2: int) -> bool:
Expand All @@ -292,15 +299,15 @@ def _check_spans_overlap(cls, start1: int, stop1: int, start2: int, stop2: int)
2.3. [x, y) and [y, y) have no overlap.
2.4. [x, z) and [y, y) are overlapping because [x, z) fully covers y point.
Despite the fact that overlapping rules are defined for 0-intervals it is not recommended \
to rely on them. If you want to get an additional annotation of the 0-interval annotation \
link one annotation to another. See `TokenAnnotation` as an example.
Despite the fact that overlapping rules are defined for 0-intervals, it is unsafe \
to rely on them. If you want to get an additional annotation of the 0-interval \
annotation, link one annotation to another. See `TokenAnnotation` for example.
:param start1: Start offset of the first span.
:param stop1: Stop offset of the first span.
:param start2: Start offset of the second span.
:param stop2: Stop offset of the second span.
:return: True if spans are overlapping else False.
:return: True if two spans overlap, otherwise False.
"""
if start1 == stop1:
if start2 == stop2:
Expand Down
19 changes: 10 additions & 9 deletions lookout/style/format/feature_extractor.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ def file_to_old_parse_file_format(file: AnnotationManager) -> Tuple[List["Virtua
Convert `AnnotationManager` instance to the deprecated output format of \
`FeatureExtractor._parse_file()`.
The function is created for backwards capability and should be removed after refactoring is \
The function exists for backward compatibility and should be removed after the refactoring is \
finished.
:param file: file annotated with `UASTAnnotation`, `PathAnnotation` and `RawTokenAnnotation`. \
Expand Down Expand Up @@ -859,13 +859,14 @@ def file_to_old_parse_file_format(file: AnnotationManager) -> Tuple[List["Virtua
def _file_to_vnodes_and_parents(file: AnnotationManager) -> Tuple[List["VirtualNode"],
Dict[int, bblfsh.Node]]:
"""
Convert `AnnotationManager` instance to the deprecated format, which is a sequence of vnodes \
and vnodes parents mapping.
Convert one `AnnotationManager` instance to the deprecated format of \
`FeatureExtractor._annotate_files()` (`_parse_vnodes()` before refactoring).
Used by `files_to_old_parse_vnodes_format` to generate old `_parse_vnodes` method output
format.
The old format is a sequence of vnodes and vnodes parents mapping. Used by
`files_to_old_parse_file_format` to generate the old `_parse_vnodes`-like output format for a
sequence of `AnnotationManager`-s.
The function is created for backwards capability and should be removed after refactoring is \
The function exists for backward compatibility and should be removed after the refactoring is \
finished.
:param file: file annotated with `Path`-, `Token`-, `Label`-, `TokenParent`- `Annotation`.
Expand Down Expand Up @@ -904,9 +905,9 @@ def files_to_old_parse_vnodes_format(
Convert a sequence of `AnnotationManager` instances to the deprecated output format of \
`FeatureExtractor._annotate_files()` (`_parse_vnodes()` before refactoring).
In addition to `_file_to_vnodes_and_parents()` it restores `node_parents` mapping.
In addition to `_file_to_vnodes_and_parents()` it provides the `node_parents` mapping.
The function is created for backwards capability and should be removed after refactoring is \
The function exists for backward compatibility and should be removed after the refactoring is \
finished.
:param files: Sequence of fully annotated files. It is expected to be the output of \
Expand All @@ -922,7 +923,7 @@ def files_to_old_parse_vnodes_format(
file_node_parents = file.get(UASTAnnotation).parents
try:
file_lines = set(file.get(LinesToCheckAnnotation).lines)
except Exception:
except KeyError:
file_lines = None
vnodes.append((file_vnodes, file_node_parents, file_lines))
vnode_parents.update(file_vnode_parents)
Expand Down

0 comments on commit d8793b4

Please sign in to comment.