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 warning to update sources if no index found #259

Closed
wants to merge 1 commit into from

Conversation

TharushiJay
Copy link
Contributor

@TharushiJay TharushiJay commented Oct 8, 2020

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/3249

Describe changes:

  • Don't automatically update the sources on list-sources if it does not exist. Instead just use the bundled version.

@inashivb
Copy link
Member

inashivb commented Oct 8, 2020

Thanks for your first contribution to the project! :)
I'll wait for Jason to respond but since its very close to completion, you can now claim another ticket.

Good job!

@TharushiJay
Copy link
Contributor Author

TharushiJay commented Oct 8, 2020 via email

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Code looks good, tests OK.

As this does change the behaviour I'd like to see more info in the commit message. The first line of the message is fine. But the body of the commit message should contain a little more detail along the lines of:

Don't automatically update the sources on list-sources if it does not exist. Instead just use the bundled version.

@TharushiJay
Copy link
Contributor Author

Thanks. Code looks good, tests OK.

As this does change the behaviour I'd like to see more info in the commit message. The first line of the message is fine. But the body of the commit message should contain a little more detail along the lines of:

Don't automatically update the sources on list-sources if it does not exist. Instead just use the bundled version.

@jasonish I updated the body of the commit message (under 'Describe changes') to reflect the details you mentioned. Is there anything else I could change to get this PR approved?

@jasonish
Copy link
Member

Thanks. Code looks good, tests OK.
As this does change the behaviour I'd like to see more info in the commit message. The first line of the message is fine. But the body of the commit message should contain a little more detail along the lines of:
Don't automatically update the sources on list-sources if it does not exist. Instead just use the bundled version.

@jasonish I updated the body of the commit message (under 'Describe changes') to reflect the details you mentioned. Is there anything else I could change to get this PR approved?

@TharushiJay You did update the pull request message. But I'd like the commit message to be updated as well. This can be done with the "reword" feature of a git rebase. If you are not familar with that feature, I'm happy to list out the commands here.

@TharushiJay
Copy link
Contributor Author

TharushiJay commented Oct 13, 2020

@TharushiJay You did update the pull request message. But I'd like the commit message to be updated as well. This can be done with the "reword" feature of a git rebase. If you are not familar with that feature, I'm happy to list out the commands here.

Sorry, my mistake! I wasn't aware of changing the 'body of the commit message'. Thank you @jasonish. I used the reword feature and I believe the body of the commit message is now modified to reflect the changes.
Please let me know if this works.

@jasonish
Copy link
Member

Close, again the code is fine, but the formatting of the commit message needs to be fixed. In general the first line should be something like:

Add warning to update sources if no index found

Usually this should be limited to 72 characters.

Then after that first line, leave one blank link then the body:

Don't automatically update the sources on list-sources 
if it does not exist. Instead just use the bundled version.

The body should be word-wrapped at 72 cols as well.

This all helps to provide a better reading experience in the "git log" output.

Thanks.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit change tweaks.

Don't automatically update the sources on list-sources
if it does not exist. Instead just use the bundled version.
@TharushiJay
Copy link
Contributor Author

Close, again the code is fine, but the formatting of the commit message needs to be fixed. In general the first line should be something like:

Add warning to update sources if no index found

Usually this should be limited to 72 characters.

Then after that first line, leave one blank link then the body:

Don't automatically update the sources on list-sources 
if it does not exist. Instead just use the bundled version.

The body should be word-wrapped at 72 cols as well.

This all helps to provide a better reading experience in the "git log" output.

Thanks.

@jasonish Thanks for the helpful advice. I've made the necessary changes. Please let me know if this works.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@inashivb inashivb mentioned this pull request Jan 7, 2022
@jasonish
Copy link
Member

jasonish commented Jan 7, 2022

Merged in master via #292, Thanks!

@jasonish jasonish closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants