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 external actvities feature #626

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

gsobczyk
Copy link

@gsobczyk gsobczyk commented Jul 27, 2020

(In reply to #493)
I've made some changes to hamster in my own repository:

  • gathering activities from an external source: Jira (but only via DBUS because I don't have a need to suggesting them - I use cinnamon applet for every operation on activities)
  • export activities as worklogs to Jira

My TODO list for external activities feature:

  • settings for external activities
  • get external activities from jira
  • cinnamon applet update
  • export flag on overview
  • exporting window
  • exporting to jira
    • only finished activities
  • exporter invoked from command line
  • add suggestions to hamster from external source (to activityentry)
  • integrate exporting with overview window
  • exporting activities via DBUS
  • export to jira item in menu - there is no need
  • zsh completion (WIP) - maybe in future

@mwilck
Copy link
Contributor

mwilck commented Aug 3, 2020

This looks very interesting, but it needs some polish. Please provide more expressive commit summaries and (at least for the non-trivial commits) descriptions of what you did, why, and how. For example, you shouldn't have three commits with the same summary test fix.

@gsobczyk gsobczyk force-pushed the add-external-actvities-feature branch 2 times, most recently from 3e56082 to a085949 Compare August 3, 2020 21:22
@gsobczyk
Copy link
Author

gsobczyk commented Aug 3, 2020

Take a look now. If there is a need to add more descriptive commit messages, please point direct commit and add example (how the commit message should look like).
I haven't seen such instruction in contributing guide (some example may be useful), but I've tried to describe every business change in commit.

@mwilck
Copy link
Contributor

mwilck commented Aug 4, 2020

Sorry, you got me wrong.

The summary (first line of the commit) should give the reader a clear idea what the commit is about. Now all your commits have the same summary, which leaves the reader completely clueless about the content of the commit unless he reads the details. Think of the summary lines as the headings. People will use git log --oneline to figure out which commits are important and how they are related to each other.

You're right, there is no explicit section about this in the contributing guide. Actually there's no common rule set that the hamster maintainers adhere to. Look at past commits to get an idea how others did it (well: hamster has had a very terse commit message style for a long time, which I personally don't like too much, so try to be better than the average hamster commit message 😄)

There are lots of general tutorials on how to write useful git commit messages on the web, e.g. here, here, or here, or the famous kernel guide Submitting Patches.
Most of this is actually common sense. Imagine someone else (or actually, yourself 2 years in the future) looking at your code and trying to figure out what you did, and try to write your commit messages such that that person has no trouble understanding what you did, how, and why.

@gsobczyk
Copy link
Author

gsobczyk commented Aug 4, 2020

Sorry, but no.
We have different practices with a commit message. :)

The summary (first line of the commit) should give the reader a clear idea what the commit is about

All commits are about "External activities". In my opinion, it's clear idea what the commit is about

try to be better than the average hamster commit message

That is why I added a description in each commit after the second line.

Imagine someone else (or actually, yourself 2 years in the future) looking at your code and trying to figure out what you did, and try to write your commit messages such that that person has no trouble understanding what you did, how, and why.

That person will look for whole commit message (using git annotate/blame) - not git log --oneline

I don't want to argue (I also don't have time for that). I thought that pull requests should check your code, not commit messages (mostly).
If you don't like such commit messages I could squash them and aggregate additional lines. I think it's the simplest resolution. What do you think about this?

@mwilck
Copy link
Contributor

mwilck commented Aug 4, 2020

That is why I added a description in each commit after the second line.

It has to be the first line. Check the references I provided. The use of the summary line is almost a universal convention.

That person will look for whole commit message (using git annotate/blame) - not git log --oneline

How do you know how other people work? I, for one, see 12x "External activitities" above, and that doesn't tell me anything about the commits unless I open every single one. I won't start a code review on a series like that. Also, I wouldn't want to see the same summary line 12x in a row in the commit history of the master branch. I'm pretty sure my fellow maintainers wouldn't, either.

"External activities" is the main title of the series and the PR, and that's where it belongs. (Btw, that title could be more descriptive too, e.g. "Add support for importing and exporting activities from Jira").

I don't want to argue (I also don't have time for that). I thought that pull requests should check your code, not commit messages (mostly).

I have better uses for my time as well. I'm not bickering, I'm just telling you what needs to be done to get this merged.

Work on comments and commit messages is just as important as work on the code itself. I know it can be frustrating because it takes a lot of extra time, but if you want to collaborate, it's a must. If you can't be bothered with that, fine with me, but don't expect this to be merged.

If you don't like such commit messages I could squash them and aggregate additional lines. I think it's the simplest resolution. What do you think about this?

That won't work. This series adds almost 1700 LoC. It has to be split in into meaningful pieces that can be reviewed separately. You can squash trivial commits like empty line removal into others, but keep the important ones separate.

- exported flag into overview window
- added export feature from overview window
- exporting worklogs to jira (only finished activities)
- added few shortcuts
Show error message when jira module isn't installed and external source is active
.. to be more descriptive
Added possibility to export activities to external source using command line
@gsobczyk gsobczyk force-pushed the add-external-actvities-feature branch from 43e9fef to fea02f6 Compare August 4, 2020 16:10
@matthijskooijman
Copy link
Member

@gsobczyk, it looks like you made a big effort to implement this and I'm happy that you want to contribute it, but in the current state, it seems unlikely that it will be merged. It is a big contribution with a lot of changes and commits, which takes quite some effort to review, polish and getting ready to merge. I personally probably won't be able to find the time in the first place, and if the commits themselves are not nicely separated and do not have verbose commit messages that help me understand what's happening, then review will be even more work and I'll be less motivated to free up time for this.

However, I do not want to invalidate your effort by ignoring or closing this, so I'm wondering how we can move this forward. Maybe you could tell us a bit more about how you designed this feature, both in terms of how it would work for the user, and how it works in technical terms? I.e. things like when are such external activitities imported (automatically, or manually?), are they copied into the local database or used as "virtual" facts, how do you configure the import sources, etc.?

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.

3 participants