doublep / logview

Emacs mode for viewing log files
GNU General Public License v3.0
153 stars 18 forks source link

Add RFC 5424 levels #32

Closed xmacex closed 4 years ago

xmacex commented 4 years ago

Hi, I added the RFC 5424 levels, as raised in #31. Please let me know if this is, or isn't interesting or if something ought to be done in some different way.

doublep commented 4 years ago

("RFC 5424" . ((error "EMERGENCY, ALERT, CRITICAL, ERROR")

I'm not sure the way you do it will work. In my code there is a list of strings, while you have a single string of comma-separated values.

Please add a regression test. There are already several tests that assert that a correct submode is selected, so you have something to base on. This will also require adding a sample log in this format --- just a few lines is enough.

Squash everything into one commit and rebase on the latest master changes, I repaired .travis.yml: currently it fails for unrelated reasons.

Also, while you are at it, please fix the typo "alises" in my code.

doublep commented 4 years ago

Ah, one more thing: mention this in README.md where it enumerates built-in submodes.

xmacex commented 4 years ago

Thanks for feedback @doublep. I believe I have remedied the bug I made yesterday (thank you for pointing it out), and made some glorious ERT tests too.

xmacex commented 4 years ago

My plan is to next provide a submode for Apache, which uses these levels (#34). If that is interesting, the plan is then to replace the Monolog submode format definition used in the tests with Apache submode.

doublep commented 4 years ago

I see in the tests you let-bind logview-additional-submodes. Shouldn't this instead be added to logview-std-submodes? I don't know how standard this format is.

If you add tests in the future, it is OK to merge similar tests into one, I don't mind tests that loop over some alist of parameters. But it's also OK to leave as it is now, up to you.

Please squash everything into one commit. Currently it's still four commits. But if you add commits unrelated to RFC 5424 later, don't squash those. I.e. basically "one new standard submode -- one commit".

doublep commented 4 years ago

I.e. in your changes you are adding new level mapping. But that on its own is not terribly useful. Shouldn't there be some standard submode that uses this mapping (I don't want to read through the complete RFC to find out)?

xmacex commented 4 years ago

Oh dear I am doing something weird with this squashing thing 😆 . Sorry I'm not used to it.

xmacex commented 4 years ago

My plan (https://github.com/doublep/logview/pull/32#issuecomment-569137238) is to also contribute some submodes (e.g. #34) – getting some levels in is a stepping stone towards that.

doublep commented 4 years ago

OK, I now understand the situation a bit. Please fix all three issues you opened in this branch and rewrite this in three commits:

This ways the tests won't need "additional submodes", they will just use the standard submode you add for #33.

Please search how you rewrite history in Git (it is generally frowned upon, but fine to do it in such an unmerged branch). I also had to do this on request when contributing to some project. If you use Magit in Emacs, use r key. Or, alternatively, just uncommit everything and then recommit in "proper" pieces with appropriate comments. Afterwards, you need to push with -f option.

xmacex commented 4 years ago

Ok, that totality should introduce RFC 5424 levels, and provide two submodes which use them, namely one for Apache error logs and another one for Monolog.

Sorry for the mess on Git side, I am not coping with the interaction of branches (I have 3, one of each feature) and squashing.

doublep commented 4 years ago

("asctime" "EEE MMM dd HH:mm:ss.SSSSSS yyyy")

Does some submode specify using precisely this format? Does it not match some national stuff handled with datetime help by default, something like English long?

xmacex wants to merge 11 commits

I will not merge 11 commits. I have been asking you to squash them from the beginning. You can squash and push-force. You can delete the branch and recreate it with the same changes in fewer commits (3 for 3 issues or even 1 for everything will do). You can even create a new PR with these changes reformulated in 3 or 1 commits. But not 11 commits please.

xmacex commented 4 years ago

("asctime" "EEE MMM dd HH:mm:ss.SSSSSS yyyy")

Does some submode specify using precisely this format?

Yes the Apache web server uses this exact date format, as documented (#34), and it is from C/C++/POSIX I believe. I wish there was ISO-8601 everywhere. I added it because Logview mode was not guessing it.

xmacex wants to merge 11 commits

I will not merge 11 commits. I have been asking you to squash them from the beginning.

Does it look fine now? One commit per issue, total 3, in this branch. I was tripping over merges/squashes/branches, but I hope it is fine now, in that regard. I apologize for the hassle and inconvenience, and thank you for patience and guidance.

doublep commented 4 years ago

Does it look fine now?

Yes, thank you. Please just do it in future without waiting for a reminder: until your changes are merged, they should be "one commit - one logical piece of changes". Of course, if something is fixed or improved after merging, squashing and rebasing should not be done anymore.

Does some submode specify using precisely this format?

Yes the Apache web server uses this exact date format, as documented (#34), and it is from C/C++/POSIX I believe.

OK, but could there be a better name referring to something then? asctime just sounds hackish to me. Or is it standard?

In the diffs I see you accidentally reindented several tests. Please undo this.

xmacex commented 4 years ago

Ok, thanks.

asctime is the only name I have seen the date format referred to, after some research into it. It is the name of the standard C function producing this format. Of course a nil would be an alternative name for it and leaving it for the user to decide. There are already three formats like that on lines 125-127. Would that be more suitable?

Indentation harmonized in tests, thanks for catching that. I must have (auto)filled the whole buffer assuming the ones already there were indented the way default Emacs would, and failed to check it. I'm learning :)

doublep commented 4 years ago

asctime is the only name I have seen the date format referred to, after some research into it. It is the name of the standard C function producing this format. Of course a nil would be an alternative name for it and leaving it for the user to decide. There are already three formats like that on lines 125-127. Would that be more suitable?

I guess if there is no real "authoritative" name, it's better to just use nil. You don't need it for references anyway.

I must have (auto)filled the whole buffer assuming the ones already there were indented the way default Emacs would, and failed to check it.

They are indented the way Emacs likes, you just hadn't evaluated the macro before reindenting I guess (macro definition influences indentation).

Please make the change with nil and I will finally merge this, it has been long enough.

doublep commented 4 years ago

Sorry that I didn't do it earlier. I didn't receive a notification for some reason (maybe it only gets sent if you comment) and then totall forgot about this PR.

Thank you for the improvements.