refactor: fix todos (#44)

* fix: fixing TODO

* chore: use enum for annotation type

* fix: use configuration exception for coverage extract

* fix: use folder to save annotation

Author: ideepu

README.md

@@ -90,9 +90,9 @@ Note: Either `GITHUB_PR_NUMBER` or `GITHUB_REF` is required. `GITHUB_PR_NUMBER`
 - `BRANCH_COVERAGE`: Show branch coverage in the report. Default is False.
 - `SKIP_COVERAGE`: Skip coverage reporting as github comment and generate only annotaions. Default is False.
 - `ANNOTATIONS_DATA_BRANCH`: The branch to store the annotations. Read more about this [here](./docs/annotations.md).
-- `ANNOTATIONS_OUTPUT_PATH`: The path where the annotaions should be stored. Should be a .json file.
+- `ANNOTATIONS_OUTPUT_PATH`: The path where the annotaions should be stored. Should be a path to folder.
 - `ANNOTATE_MISSING_LINES`: Whether to annotate missing lines in the coverage report. Default is False.
-- `ANNOTATION_TYPE`: The type of annotation to use for missing lines. Default is 'warning'.
+- `ANNOTATION_TYPE`: The type of annotation to use for missing lines. 'notice' or 'warning' or 'error'. Default is 'warning'.
 - `MAX_FILES_IN_COMMENT`: The maximum number of files to include in the coverage report comment. Default is 25.
 - `COMPLETE_PROJECT_REPORT`: Whether to include the complete project coverage report in the comment. Default is False.
 - `COVERAGE_REPORT_URL`: URL of the full coverage report to mention in the comment.

codecov/config.py

@@ -5,11 +5,10 @@
 import inspect
 import pathlib
 from collections.abc import MutableMapping
+from enum import Enum
 from typing import Any, Callable
 
-from codecov.exceptions import InvalidAnnotationType, MissingEnvironmentVariable
-
-# TODO: Rename this file to config.py
+from codecov.exceptions import MissingEnvironmentVariable
 
 
 def path_below(path_str: str | pathlib.Path) -> pathlib.Path:
@@ -26,8 +25,14 @@ def str_to_bool(value: str) -> bool:
     return value.lower() in ('1', 'true', 'yes')
 
 
+class AnnotationType(Enum):
+    NOTICE = 'notice'
+    WARNING = 'warning'
+    ERROR = 'error'
+
+
 # pylint: disable=invalid-name, too-many-instance-attributes
-@dataclasses.dataclass
+@dataclasses.dataclass(kw_only=True)
 class Config:
     """This object defines the environment variables"""
 
@@ -44,8 +49,7 @@ class Config:
     BRANCH_COVERAGE: bool = False
     SKIP_COVERAGE: bool = False
     ANNOTATE_MISSING_LINES: bool = False
-    # TODO: Make it enum
-    ANNOTATION_TYPE: str = 'warning'
+    ANNOTATION_TYPE: AnnotationType = AnnotationType.WARNING
     ANNOTATIONS_OUTPUT_PATH: pathlib.Path | None = None
     ANNOTATIONS_DATA_BRANCH: str | None = None
     MAX_FILES_IN_COMMENT: int = 25
@@ -88,12 +92,8 @@ def clean_debug(cls, value: str) -> bool:
         return str_to_bool(value)
 
     @classmethod
-    def clean_annotation_type(cls, value: str) -> str:
-        if value not in {'notice', 'warning', 'error'}:
-            raise InvalidAnnotationType(
-                f'The annotation type {value} is not valid. Please choose from notice, warning or error'
-            )
-        return value
+    def clean_annotation_type(cls, value: str) -> AnnotationType:
+        return AnnotationType(value)
 
     @classmethod
     def clean_github_pr_number(cls, value: str) -> int:
@@ -105,7 +105,10 @@ def clean_coverage_path(cls, value: str) -> pathlib.Path:
 
     @classmethod
     def clean_annotations_output_path(cls, value: str) -> pathlib.Path:
-        return pathlib.Path(value)
+        path = pathlib.Path(value)
+        if path.exists() or path.is_dir():
+            return path
+        raise ValueError
 
     # We need to type environ as a MutableMapping because that's what
     # os.environ is, and `dict[str, str]` is not enough

codecov/coverage/base.py

@@ -9,6 +9,7 @@
 from collections import deque
 from collections.abc import Sequence
 
+from codecov.exceptions import ConfigurationException
 from codecov.log import log
 
 
@@ -86,16 +87,15 @@ def compute_coverage(self, num_covered: int, num_total: int) -> decimal.Decimal:
         return decimal.Decimal(num_covered) / decimal.Decimal(num_total)
 
     def get_coverage_info(self, coverage_path: pathlib.Path) -> Coverage:
-        # TODO: Write a custom exception here and handle it in the main script
         try:
             with coverage_path.open() as coverage_data:
                 json_coverage = json.loads(coverage_data.read())
-        except FileNotFoundError:
+        except FileNotFoundError as exc:
             log.error('Coverage report file not found: %s', coverage_path)
-            raise
-        except json.JSONDecodeError:
+            raise ConfigurationException from exc
+        except json.JSONDecodeError as exc:
             log.error('Invalid JSON format in coverage report file: %s', coverage_path)
-            raise
+            raise ConfigurationException from exc
 
         return self.extract_info(data=json_coverage)
 

codecov/exceptions.py

@@ -50,10 +50,6 @@ class MissingEnvironmentVariable(ConfigurationException):
     pass
 
 
-class InvalidAnnotationType(ConfigurationException):
-    pass
-
-
 class TemplateBaseException(CoreBaseException):
     pass
 

codecov/github.py

@@ -120,9 +120,8 @@ def post_comment(self, contents: str, marker: str) -> None:
             log.error('Comment exceeds allowed size(65536)')
             raise CannotPostComment
 
-        # TODO: Try and see if you can refactor this to use single request
-        # issue_comments_path.patch(body=contents)
-        # TODO: Make scope only for pull request not for issues
+        # Pull request review comments are comments made on a portion of the unified diff during a pull request review.
+        # Issue comments are comments on the entire pull request. We need issue comments.
         issue_comments_path = self.client.repos(self.repository).issues(self.pr_number).comments
         comments_path = self.client.repos(self.repository).issues.comments
         for comment in issue_comments_path.get():

codecov/github_client.py

@@ -1,5 +1,7 @@
 from __future__ import annotations
 
+from typing import Any
+
 import httpx
 
 from codecov.exceptions import ApiError, ConfigurationException, Conflict, Forbidden, NotFound, ValidationFailed
@@ -80,7 +82,7 @@ def __getattr__(self, attr):
 
     def _http(self, method: str, path: str, *, use_bytes: bool = False, use_text: bool = False, **kw):
         _method = method.lower()
-        requests_kwargs = {}
+        requests_kwargs: dict[Any, Any] = {}
         headers = kw.pop('headers', {})
         if _method == 'get' and kw:
             requests_kwargs = {'params': kw}
@@ -106,14 +108,16 @@ def _http(self, method: str, path: str, *, use_bytes: bool = False, use_text: bo
         try:
             response.raise_for_status()
         except httpx.HTTPStatusError as exc:
-            # TODO: Use `match` Statement
-            cls: type[ApiError] = {
-                403: Forbidden,
-                404: NotFound,
-                409: Conflict,
-                422: ValidationFailed,
-            }.get(exc.response.status_code, ApiError)
-
-            raise cls(str(contents)) from exc
+            exc_cls = ApiError
+            match exc.response.status_code:
+                case 403:
+                    exc_cls = Forbidden
+                case 404:
+                    exc_cls = NotFound
+                case 409:
+                    exc_cls = Conflict
+                case 422:
+                    exc_cls = ValidationFailed
+            raise exc_cls(str(contents)) from exc
 
         return contents

codecov/main.py

@@ -5,7 +5,7 @@
 from codecov.config import Config
 from codecov.coverage import PytestCoverage
 from codecov.coverage.base import Coverage, DiffCoverage
-from codecov.exceptions import CoreProcessingException, MissingMarker, TemplateException
+from codecov.exceptions import ConfigurationException, CoreProcessingException, MissingMarker, TemplateException
 from codecov.github import Github
 from codecov.github_client import GitHubClient
 from codecov.log import log, setup as log_setup
@@ -51,7 +51,12 @@ def run(self):
 
     def _process_coverage(self):
         log.info('Processing coverage data')
-        coverage = self.coverage_module.get_coverage_info(coverage_path=self.config.COVERAGE_PATH)
+        try:
+            coverage = self.coverage_module.get_coverage_info(coverage_path=self.config.COVERAGE_PATH)
+        except ConfigurationException as e:
+            log.error('Error processing coverage data.')
+            raise CoreProcessingException from e
+
         if self.config.BRANCH_COVERAGE:
             coverage = diff_grouper.fill_branch_missing_groups(coverage=coverage)
         added_lines = self.coverage_module.parse_diff_output(diff=self.github.pr_diff)
@@ -64,7 +69,7 @@ def _process_pr(self):
             log.info('Skipping coverage report generation.')
             return
 
-        log.info('Generating comment for PR')
+        log.info('Generating comment for PR #%s', self.github.pr_number)
         marker = template.get_marker(marker_id=self.config.SUBPROJECT_ID)
         files_info, count_files, changed_files_info = template.select_changed_files(
             coverage=self.coverage,
@@ -99,18 +104,15 @@ def _process_pr(self):
             )
         except MissingMarker as e:
             log.error(
-                'Marker not found. This error can happen if you defined a custom comment '
-                "template that doesn't inherit the base template and you didn't include "
-                '``{{ marker }}``. The marker is necessary for this action to recognize '
-                "its own comment and avoid making new comments or overwriting someone else's "
-                'comment.'
+                '``{{ %s }}`` marker not found. The marker is necessary for this action to recognize '
+                "its own comment and avoid making new comments or overwriting someone else's comment.",
+                marker,
             )
             raise CoreProcessingException from e
         except TemplateException as e:
             log.error(
                 'There was a rendering error when computing the text of the comment to post '
-                "on the PR. Please see the traceback, in particular if you're using a custom "
-                'template.'
+                'on the PR. Please see the traceback for more information.'
             )
             raise CoreProcessingException from e
 
@@ -125,7 +127,7 @@ def _generate_annotations(self):
         log.info('Generating annotations for missing lines.')
         annotations = diff_grouper.get_diff_missing_groups(coverage=self.coverage, diff_coverage=self.diff_coverage)
         formatted_annotations = groups.create_missing_coverage_annotations(
-            annotation_type=self.config.ANNOTATION_TYPE,
+            annotation_type=self.config.ANNOTATION_TYPE.value,
             annotations=annotations,
         )
 
@@ -136,7 +138,7 @@ def _generate_annotations(self):
             )
             formatted_annotations.extend(
                 groups.create_missing_coverage_annotations(
-                    annotation_type=self.config.ANNOTATION_TYPE,
+                    annotation_type=self.config.ANNOTATION_TYPE.value,
                     annotations=branch_annotations,
                     branch=True,
                 )
@@ -155,12 +157,13 @@ def _generate_annotations(self):
         print(reset, end='')
 
         # Save to file
-        # TODO: Take the folder path instead of the file path
+        file_name = f'{self.github.pr_number}-annotations.json'
         if self.config.ANNOTATIONS_OUTPUT_PATH:
-            log.info('Writing annotations to file.')
-            with self.config.ANNOTATIONS_OUTPUT_PATH.open('w+') as annotations_file:
+            log.info('Writing annotations to file %s', file_name)
+            with self.config.ANNOTATIONS_OUTPUT_PATH.joinpath(file_name).open('w+') as annotations_file:
                 json.dump(formatted_annotations, annotations_file, cls=groups.AnnotationEncoder)
 
+        # Write to branch
         if self.config.ANNOTATIONS_DATA_BRANCH:
             log.info('Writing annotations to branch.')
             self.github.write_annotations_to_branch(annotations=formatted_annotations)

docs/annotations.md

@@ -8,8 +8,8 @@ By default, these annotations are written to the console, but you can also choos
 
 ## Storing Annotations
 
-1. **To a File**:
-   - Set the file path in `ANNOTATIONS_OUTPUT_PATH`.
+1. **To a Folder**:
+   - Set the folder path in `ANNOTATIONS_OUTPUT_PATH`.
 
 2. **To a Branch**:
    - Set the branch name in `ANNOTATIONS_DATA_BRANCH`.

tests/conftest.py

@@ -92,14 +92,14 @@ def _(code: str, has_branches: bool = True) -> Coverage:
                 )
             if any(
                 x in line
-                for x in {
+                for x in (
                     'line covered',
                     'line missing',
                     'line excluded',
                     'branch covered',
                     'branch missing',
                     'branch partial',
-                }
+                )
             ):
                 coverage_obj.files[current_file].info.num_statements += 1
                 coverage_obj.info.num_statements += 1

tests/coverage/test_base.py

@@ -10,6 +10,7 @@
 
 from codecov.coverage import Coverage, DiffCoverage, FileDiffCoverage
 from codecov.coverage.base import BaseCoverage
+from codecov.exceptions import ConfigurationException
 
 
 class BaseCoverageDemo(BaseCoverage):
@@ -54,10 +55,10 @@ def test_get_coverage_info(self, coverage_json):
 
         with patch('pathlib.Path.open') as mock_open:
             mock_open.return_value.__enter__.return_value.read.return_value = b'invalid json'
-            with pytest.raises(json.JSONDecodeError):
+            with pytest.raises(ConfigurationException):
                 BaseCoverageDemo().get_coverage_info(pathlib.Path(tempfile.mkstemp(suffix='.json')[1]))
 
-        with pytest.raises(FileNotFoundError):
+        with pytest.raises(ConfigurationException):
             BaseCoverageDemo().get_coverage_info(pathlib.Path('path/to/file.json'))
 
     @pytest.mark.parametrize(