-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Format Devices #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good practice when you are programming in Python is adding the type of the parameters. For example:
def write_statistics_packets(self, state):
"""Write the packet statistics."""
Params:
+state (dict): state of blablabl...
Also, there are a lot of methods where the parameters are not in the documentation. Example:
def dict_regex_search(content, regex):
"""Apply the regex over all the fields of the dictionary."""
CODE
In this case, we should document what are content and regex parameters and what is their "mission"
def write_message(self, content, state): | ||
"""Write the message. | ||
|
||
The content argument is a dictionary with at least 'description' item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is not a good idea to explain the "content" param in this way. Please, follow the same style as in the other functions
@@ -16,82 +16,104 @@ | |||
"""Logger functions. | |||
|
|||
The module contains functions to log the new messages. | |||
The 'content' dictionary is defined in the FormatDevice class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about how to describe this param but I think that this is not correct
class MarkdownFormatDevice(FormatDevice): | ||
"""Format device for Markdown. | ||
|
||
Functions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods
The module contains the abstract class representation for a formatter of the | ||
parsed logs. | ||
|
||
Classes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this comment it is not necessary: we only have a class in this file (with the same name of the class).
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
"""Package LogParser.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package LogParser or LogParser Packgage?
Thanks for the feedback!
I think that you are right. I have been duplicating the documentation in all the headers of the packages and classes and this is very time consuming and useless.
I was following some best-practices from Google but as said it could not be the best idea. At the end, we should write documentation that looks good when using the `help()` function and that follows the community practices.
However, making these changes in this PR will make these package to not follow the current *practices* (inconsistency with other packages) and it will cause many conflicts for the existing PR. I will create a new issue to make these changes after merging the current branches.
|
I have created #11 to track the documentation discussion. Do you agree to make these changes in other PR? If so, is there any change pending in this PR? |
Perfect! |
Implement a new device that allows to show the parsed output in different format. The default and current implementation is Markdown but in the future we could implement XML or JSON devices.