#55 - Chapter line formatting - authors #74
Conversation
- Add limitation to avoid of usage black to tests. - Removed workflow copying Release notes from PR to Issue as no more supported. - Updated README.md to show example of row formatting as build-in feature and provide list of supported keywords.
- Fixed structure of test folder.
- Improved validation logic to support difference keywords for issue, pr and commit format rows.
- Improved README to provide better description about format keyword and data sources.
|
Release notes:
|
There was a problem hiding this comment.
This is part of a partial review. Please comment or implement suggestions mentioned in the comments. During my work on liv-doc, I found as well one more issue.
- in a test.yml workflow, there is an issue in unit-test. You check cov with pytest. There is no need to have an html report. You can look at liv-doc for inspiration. After pytest cov check, you check cov again with coverage tool and export it to xml. This is not needed at all, since we just check if cov is above or below 80 %.
- if we agree on the point above, you should also remove coverage==7.5.2 from your requirements.txt
| - [Handling Issue Mentioned By Multiple PRs](#handling-issue-mentioned-by-multiple-prs) | ||
| - [No Release Notes Found](#no-release-notes-found) | ||
| - [Issue, Pull Request or Commit Row formatting](#issue-pull-request-or-commit-row-formatting) | ||
| - [Supported row format keywords](#supported-row-format-keywords) |
There was a problem hiding this comment.
There are some bugs, I found during review (README.md), that are already merged. Please find this comment as a block of suggestions, that may occur on more than just one place.
- Please reconsider and make a statement about what levels of headers you want to visualize in POC. In my opinion here you go too deep visualizing h5. There are many h3 in inputs section, which are not seen in PoC. It doesn't look compact.
- This h5 doesn't really have to be extra header at all. Because there is just one header in the section at the time.
- I think that best practice is to have a new header level only, whenever you need @ or more lower level of headings. Easier to see in practice. You have h2 (## Outputs) and then you have just one h3 (### Supported Row Types) and after 3 times h4. I don't see a reason to have this one h3. There is not a single sentence anyway.
- The same issue in h5 (##### Supported row format keywords). It is not needed IMHO.
- look at line 99 =
### Feature controls, it should be higher level of a header based on the context around. - look at line 150 =
#### Default. I am suggesting to change it to Default, since there is no need to have it as a header. Same at line 167. - after Black Expected Output example there is an extra blank line
- add information, that pylint and black tool exclude tests/ file
There was a problem hiding this comment.
This part will be reworked, including a change of chapter meaning.
This text will not be reused.
Keeping not Resolved to check again after new data structure will be implemented.
| self.chapters[MERGED_PRS_LINKED_TO_NOT_CLOSED_ISSUES].add_row(nr, records[nr].to_chapter_row()) | ||
| self.used_record_numbers.append(nr) | ||
| else: | ||
| self.chapters[OTHERS_NO_TOPIC].add_row(nr, records[nr].to_chapter_row()) | ||
| self.used_record_numbers.append(nr) | ||
|
|
||
| def __populate_closed_issues(self, record: Record, nr: int) -> None: | ||
| def __populate_isolated_commits(self, r: Record, nr: str) -> None: |
There was a problem hiding this comment.
Missing method docstring.
There was a problem hiding this comment.
Present only in this branch.
| self.used_record_numbers.append(nr) | ||
|
|
||
| def __is_row_present(self, nr: int) -> bool: | ||
| def __is_row_present(self, nr: int | str) -> bool: |
There was a problem hiding this comment.
Missing method docstring.
|
|
||
| def create_record_for_issue(r: Repository, i: Issue): | ||
| records[i.number] = Record(r, i) | ||
| def create_record_for_issue(i: Issue): |
There was a problem hiding this comment.
Check missing method docstrings.
| @@ -23,7 +23,7 @@ | |||
| from functools import wraps | |||
| from typing import Callable, Optional, Any | |||
| from github import GithubException | |||
| from requests.exceptions import Timeout, RequestException, ConnectionError as RequestsConnectionError | |||
| from requests.exceptions import Timeout, RequestException, ConnectionError | |||
There was a problem hiding this comment.
ConnectionError is a built in exception, you don't have to import it.
| @@ -0,0 +1,2 @@ | |||
| class NotSupportedException(Exception): | |||
There was a problem hiding this comment.
Add all the documentation needed for this new module.
There was a problem hiding this comment.
This class is present only this branch.
|
This branch will not be used in future. |
Closes #55
Closes #56