Refactoring/13 documentation and formatting #30

MobiTikula · 2024-08-14T15:45:52Z

This PR has a goal to add to the project:

method documentation
module documentation
class documentation
poetry for environment management
black format tool and script formatting

Closes #13

…e at constants.py.

miroslavpojer

pull
run
code review
I would propose creation of python project team format rules in cps-hadnbook.
I am missing mentioned black-poetry output.

main.py

living_documentation_generator/generator.py

living_documentation_generator/github_projects.py

living_documentation_generator/model/project_issue.py

living_documentation_generator/model/project_status.py

living_documentation_generator/utils/decorators.py

living_documentation_generator/utils/utils.py

lsulak · 2024-08-20T13:24:07Z

living_documentation_generator/generator.py

+        __repositories (list[ConfigRepository]): List of config repositories to fetch from.
+        __project_state_mining_enabled (bool): Switch indicating if project state mining is enabled.
+        __output_path (str): The directory where the markdown pages will be stored.
+    """

    PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__))

    ISSUE_PAGE_TEMPLATE_FILE = os.path.join(PROJECT_ROOT, "..", "templates", "issue_detail_page_template.md")


considering using os.pardir instead of ".."

Great advice, change implemented in: 5db1778

lsulak · 2024-08-20T13:26:32Z

living_documentation_generator/generator.py

+    A class representing the Living Documentation Generator.
+
+    Attributes:
+        __github_instance (GitHub): The GitHub instance used for authentication and requests.


perhaps it's a bit redundant to have the attributes documented here and then also individually below. I'd just probably have the ones below, on the 'function' level

also documenting types is redundant if you are using typing :)

This comment led us into a conversation about the doc in all of our future QA projects, where agreed on some rules, that we will follow. All the documentation is updated.

lsulak · 2024-08-21T08:08:12Z

living_documentation_generator/generator.py

            issue_page_detail_template = f.read()

-        with open(LivingDocumentationGenerator.INDEX_PAGE_TEMPLATE_FILE, 'r', encoding='utf-8') as f:
+        with open(LivingDocumentationGenerator.INDEX_PAGE_TEMPLATE_FILE, "r", encoding="utf-8") as f:


regarding these 2 with statements: what if the file was not opened successfully for whatever reason? Then issue_page_detail_template and issue_index_page_template wouldn't be available and the program would crash. Perhaps consider to either move operations working with them into the individual with scope, or initialize the variable above as None

Logic was updated at: 5db1778

lsulak · 2024-08-21T08:18:46Z

living_documentation_generator/generator.py

-                           " Linked to project | Project status | Issue URL |\n"
-                           "|-------------------|-----------------|------------------------|"
-                           "-------------------|----------------|-----------|\n")
+            issue_table = (


I'd consider extracting this to some constants.py or at least on top of the file as global constant perhaps - it's good to have visibility over these things

Issue tables for index page are now moved into constants.py file (5db1778).

lsulak

Note: I only reviewed the generator.py

I quite like the way you progressed in Python!! Now, the next evolution of your engineering skills can be to try some testing exercises. Maybe even, when / if that makes sense, to try TDD (Test Driven Development - be careful, this is not a silver bullet in my opinion, but can be incredibly handy sometimes :) )

https://martinfowler.com/bliki/TestDrivenDevelopment.html
https://en.wikipedia.org/wiki/Test-driven_development
https://www.amazon.com/Test-Driven-Development-Kent-Beck/dp/0321146530 (if you want to take it that far...can be good to read at some point in your engineering career I'd say)

The point is, to cover some of the logic you wrote with unit tests - it actually is a 'documentation' for the 'usage' I would say, beyond just testing. Some people on my team are actually reading tests first, when reviewing someone else's PRs, so that they have good understanding on inputs and outputs and/or side effects of a given functionality, and that helps massively.

But good work, I like how it can be read from top to bottom almost like a book (so like in the book, you open it and tehre is table of content - in your case, method generate that implements nothing, but only calls high level operations, and then you go to individual operations - chapters of the book, for more details), there is a proper logging, documentation, typing, relatively good responsibility levels & sizes of functions :) keep up with good work

MobiTikula · 2024-08-30T09:51:31Z

Release notes

method documentation
module documentation
class documentation
implementing Black format tool
all python code formatted
README.md updated with using new tools

MobiTikula · 2024-08-30T09:55:41Z

Poetry was not implemented to the POC and is moved into separate SPIKE issue.

miroslavpojer · 2024-09-01T06:59:38Z

Poetry was not implemented to the POC and is moved into separate SPIKE issue.

Place here a link to issue, please.

miroslavpojer

pull
code review
run

You have done excellent work here.

I may have one idea.

Are we expecting the code ready for review to be "blacked" already? I mean, would black not report any change in any file?
If the previous bullet is valid. We can prepare a CI check action, which will try to run black again and check if there were some changes in the code. If done, we can report a problem.

miroslavpojer · 2024-09-01T08:07:24Z

living_documentation_generator/action_inputs.py

-    get_action_input,
-    make_absolute_path,
-)
+from living_documentation_generator.utils.utils import get_action_input, make_absolute_path


From how many import items are required ()?

Whenever line is more than 120 char. The rules are still same. BUT If we talking about for example constants, which can increase during the time, I also think is alright to put them under each other. Black way of formatting is to reduce as many extra lines as possible. But it won't change your code, if you choose to put just two of your imports under each other.

MobiTikula · 2024-09-02T07:41:07Z

Poetry was not implemented to the POC and is moved into separate SPIKE issue.

Place here a link to issue, please.

Here is the link for the SPIKE issue #32.

MobiTikula · 2024-09-02T07:57:03Z

pull

code review

run

You have done excellent work here.

I may have one idea.

Are we expecting the code ready for review to be "blacked" already? I mean, would black not report any change in any file?

If the previous bullet is valid. We can prepare a CI check action, which will try to run black again and check if there were some changes in the code. If done, we can report a problem.

I also had that idea to implement Black as a CI workflow. With few adjustments we can make it real! With this workflow, we can make sure that all versions of projects have the same format.

lsulak · 2024-09-02T09:21:01Z

.github/workflows/static_analysis_and_tests.yml

+      - name: Check code format with Black
+        id: check-format
+        run: |
+          black --check $(git ls-files '*.py')


why it is here also? It's duplicated, perhaps it's enough if black step is in the job analysis and then you can remove code-format-check completely - unless there is a good reason for its existence?

This block of code was not supposed to be there twice. Our idea is to have black tool as separate workflow, since it has different goal than pylint. Changed in: fdafc07.

It was my idea. There are two possible reasons for job failure.

ok then, as the duplication was removed now, I'll approve as there is nothing else at the moment :) GJ!

lsulak · 2024-09-02T09:23:42Z

README.md

+```
+
+## Run Black Tool Locally
+This project uses the Black tool for code formatting.


Suggested change

This project uses the Black tool for code formatting.

This project uses the [Black](https://github.com/psf/black) tool for code formatting.

Change as well as for Pylint tool: fdafc07

…-formatting' into refactoring/13-Documentation-and-formatting

miroslavpojer

I see no more "problems".

miroslavpojer · 2024-09-02T09:55:08Z

.github/workflows/static_analysis_and_tests.yml

+      - name: Check code format with Black
+        id: check-format
+        run: |
+          black --check $(git ls-files '*.py')


It was my idea. There are two possible reasons for job failure.

MobiTikula added 13 commits August 2, 2024 14:35

Implementing suggested bug fixes to the POC.

8e1b161

Updating README.md with new section Expected Output.

0a0498d

Removing Todos from the project. Improving action input logic.

96a1a5a

action.yml and README.md files updated for current state of POC.

f06dc31

Constants handling improved to have all project constants on one plac…

be75de8

…e at constants.py.

First phase of pylint refactoring.

f6802f3

Second phase of linting the project.

d678e81

Third phase of linting the project.

0677c53

Last phase of linting the project.

7c0f8c1

Code format checked with BLACK: The Uncompromising Code Formatter.

b3e3d99

Adding the class docstrings documentation to the project.

eb6b679

Adding the module docstrings documentation to the project.

e465f58

Method docstring documentation added to the project.

a22f30e

MobiTikula added the refactoring Improving code quality, paying off tech debt, aligning APIs label Aug 14, 2024

MobiTikula self-assigned this Aug 14, 2024

MobiTikula requested review from miroslavpojer and Zejnilovic as code owners August 14, 2024 15:45

MobiTikula changed the base branch from master to refactoring/23-Introduce-code-format-check-and-linter-check August 14, 2024 15:46

MobiTikula linked an issue Aug 14, 2024 that may be closed by this pull request

Refactoring - Documentation and formatting #13

Closed

miroslavpojer requested a review from lsulak August 20, 2024 10:59

miroslavpojer reviewed Aug 20, 2024

View reviewed changes

lsulak reviewed Aug 20, 2024

View reviewed changes

lsulak reviewed Aug 21, 2024

View reviewed changes

Base automatically changed from refactoring/23-Introduce-code-format-check-and-linter-check to master August 28, 2024 08:12

MobiTikula added 3 commits August 28, 2024 10:55

Solving merge conflicts.

c39b54b

Docstrings update for liv-generator-model.

0b29d27

Docstrings update for liv-generator-utils.

7708806

MobiTikula added 3 commits August 28, 2024 13:39

Docstrings update based on the new standards.

d5e2d21

Formating project with black tool.

b9e4d68

Review comments implemented into POC.

5db1778

MobiTikula mentioned this pull request Aug 30, 2024

#64 - Introduce the pylint into project AbsaOSS/generate-release-notes#65

Merged

MobiTikula added 3 commits August 30, 2024 11:37

Updated README.md for using a Black formatting tool.

0bdbf9b

Updated README.md for using a Black formatting tool.

c807e2c

Updated README.md for using tools.

9c49482

MobiTikula requested review from miroslavpojer and lsulak August 30, 2024 09:52

miroslavpojer reviewed Sep 1, 2024

View reviewed changes

MobiTikula and others added 5 commits September 2, 2024 10:02

Comments suggestions added.

b70f0e9

Black workflow added.

56d941e

CI black wrong commit.

a9def3f

CI black workflow implemented.

830ccc6

Update static_analysis_and_tests.yml

3cdfef6

lsulak reviewed Sep 2, 2024

View reviewed changes

MobiTikula added 2 commits September 2, 2024 11:48

Bug fixes.

fdafc07

Merge remote-tracking branch 'origin/refactoring/13-Documentation-and…

1324bc6

…-formatting' into refactoring/13-Documentation-and-formatting

miroslavpojer approved these changes Sep 2, 2024

View reviewed changes

lsulak approved these changes Sep 2, 2024

View reviewed changes

MobiTikula merged commit d2249ea into master Sep 2, 2024
4 checks passed

MobiTikula deleted the refactoring/13-Documentation-and-formatting branch September 2, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring/13 documentation and formatting #30

Refactoring/13 documentation and formatting #30

MobiTikula commented Aug 14, 2024 •

edited

Loading

miroslavpojer left a comment •

edited

Loading

lsulak Aug 20, 2024

MobiTikula Aug 29, 2024

lsulak Aug 20, 2024

lsulak Aug 20, 2024

MobiTikula Aug 29, 2024

lsulak Aug 21, 2024

MobiTikula Aug 29, 2024

lsulak Aug 21, 2024

MobiTikula Aug 29, 2024

lsulak left a comment •

edited

Loading

MobiTikula commented Aug 30, 2024

MobiTikula commented Aug 30, 2024

miroslavpojer commented Sep 1, 2024

miroslavpojer left a comment

miroslavpojer Sep 1, 2024

MobiTikula Sep 2, 2024

MobiTikula commented Sep 2, 2024 •

edited

Loading

MobiTikula commented Sep 2, 2024

lsulak Sep 2, 2024

MobiTikula Sep 2, 2024

miroslavpojer Sep 2, 2024

lsulak Sep 2, 2024

lsulak Sep 2, 2024

MobiTikula Sep 2, 2024

miroslavpojer left a comment

miroslavpojer Sep 2, 2024

	This project uses the Black tool for code formatting.
	This project uses the [Black](https://github.com/psf/black) tool for code formatting.

Refactoring/13 documentation and formatting #30

Refactoring/13 documentation and formatting #30

Conversation

MobiTikula commented Aug 14, 2024 • edited Loading

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak left a comment • edited Loading

Choose a reason for hiding this comment

MobiTikula commented Aug 30, 2024

MobiTikula commented Aug 30, 2024

miroslavpojer commented Sep 1, 2024

miroslavpojer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MobiTikula commented Sep 2, 2024 • edited Loading

MobiTikula commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavpojer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MobiTikula commented Aug 14, 2024 •

edited

Loading

miroslavpojer left a comment •

edited

Loading

lsulak left a comment •

edited

Loading

MobiTikula commented Sep 2, 2024 •

edited

Loading