#1757 Add description for tags
Merged by pingou. Opened by cep.
cep/pagure description_for_issues_tags  into  master

Download 1757.patch

Implements feature https://pagure.io/pagure/issue/1756

:information_source: Not up for review yet.

It works, but I'll have to write/update unit tests. Please test and comment! :)

This doesn't look very informative does it ? :)

This seems to happen if there's some corruption/tampering of the form. So, will something like "Your request seemd to be incomplete. Please try again." be more apt?

Yeah something on those lines if we could tell which one is missing that will be icing on the cake :)

1 new commit added

  • Fix tests after description for tags feature

1 new commit added

  • More helpful error messages for creating tags

@farhaan There's your cake and icing :)

PR is now up for review! :boom: :star2:

I am nitpicking here but try to start a comment with caps :)

Below we are checking this right , One or more tag field missing doesn't give as deep insights as the below two condition , so IMHO we should keep two conditions len_colors != len_tags and len_tag_descriptions != len_tags and appending message as it is.

Do you think we are using the length that heavily that it should be assigned to a variable? and len(tags) is equally descriptive as len_tags , isn't it ?

No docstring?

Likely needs an update :)

Why not just properly defining the label?

This should likely be split

I like that we pay attention to detail. Will take care of it :)

Yes, I figured three function calls instead of many would be economical, time wise. What do you suggest?

Removing this won't account for the case where we might have a lost/empty tag field. I think we should keep this check. Or do you have an alternative idea?

Yes. Hopefully, it's seamless :)
Edit: Got what you meant. On it!

Can you please give an example?

https://pagure.io/pagure/blob/master/f/pagure/forms.py#_139-140 might give you a clue :)

rebased

I think I addressed all the issues mentioned. Let's please have another round of reviews :)

This is a purely personal preference, but I like it better when we split before the keyword, so foo \ and bar \ or baz

You may want to run pep8 on this, as I think it will ask for this line to be aligned with the opening bracket

This change is no longer needed no?

Ah, I missed that. Thanks!

Couple of nitpicks but this is looking good :)

8 new commits added

  • Minor fixes for feature: Description for Tags
  • More helpful error messages for creating tags
  • Fix tests after description for tags feature
  • Show proper labels in edit_tag page
  • Extend render_bootstrap_field macro with field_label
  • Add DB revision for tag description column
  • Fix tag creation breaking on empty description
  • Add Description for Tags

Nothing standing out for me when looking at it, I'll test it before merging :)

This will need to be updated after the rebase, cf alembic history for more info :)

Might be cool to include this description in issues.html next to the issue titles on the list

Maybe: tag.tag_description or '' to avoid showing 'None'

Except for these few comments this is looking good and working fine from local testing :)

I thought about that too, but felt it would be unnecessary / a bit annoying. I'm not sure, but I'll include this if you feel its needed. What do you think?

I think this will be taken care of by https://pagure.io/pagure/pull-request/1757#4_5 right? Or am I wrong?

The advantages of the list of issues is that people see all/most of the tags at once, while on the ticket you only see a potentially very limited subset

Nope, and not on existing tags :)

How do you feel about doing this for the list of all tags, displayed above the issues list?

Will fix this :)

I thought about this as well but they already have a title, so not sure what to do here :s

Alright. Let's add it to the tags at the end of issue titles for now :)

rebased

Looks good to me!

Pull-Request has been merged by pingou

Metadata