Skip to content
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

Add RFC 5424 levels #32

Merged
merged 3 commits into from
Jan 19, 2020
Merged

Add RFC 5424 levels #32

merged 3 commits into from
Jan 19, 2020

Conversation

xmacex
Copy link
Contributor

@xmacex xmacex commented Dec 25, 2019

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
Copy link
Owner

doublep commented Dec 26, 2019

("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
Copy link
Owner

doublep commented Dec 26, 2019

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

@xmacex
Copy link
Contributor Author

xmacex commented Dec 26, 2019

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
Copy link
Contributor Author

xmacex commented Dec 26, 2019

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
Copy link
Owner

doublep commented Dec 27, 2019

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
Copy link
Owner

doublep commented Dec 27, 2019

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
Copy link
Contributor Author

xmacex commented Dec 27, 2019

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

@xmacex
Copy link
Contributor Author

xmacex commented Dec 27, 2019

My plan (#32 (comment)) is to also contribute some submodes (e.g. #34) – getting some levels in is a stepping stone towards that.

@doublep
Copy link
Owner

doublep commented Dec 28, 2019

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
Copy link
Contributor Author

xmacex commented Dec 29, 2019

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
Copy link
Owner

doublep commented Jan 4, 2020

("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
Copy link
Contributor Author

xmacex commented Jan 4, 2020

("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
Copy link
Owner

doublep commented Jan 4, 2020

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
Copy link
Contributor Author

xmacex commented Jan 4, 2020

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
Copy link
Owner

doublep commented Jan 4, 2020

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 doublep merged commit 5d78271 into doublep:master Jan 19, 2020
@doublep
Copy link
Owner

doublep commented Jan 19, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants