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

A file template is added for a Jenkins Jelly View #112

Merged
merged 5 commits into from
Oct 11, 2022
Merged

A file template is added for a Jenkins Jelly View #112

merged 5 commits into from
Oct 11, 2022

Conversation

julieheard
Copy link
Contributor

@julieheard julieheard commented Oct 4, 2022

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira - Contribute Jelly File Template #41
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

I have followed the tutorials here https://plugins.jetbrains.com/docs/intellij/providing-file-templates.html to start making file templates. This is for issue here - #41

@duemir duemir self-requested a review October 5, 2022 01:59
Copy link
Member

@duemir duemir left a comment

Choose a reason for hiding this comment

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

Thank you @julieheard ! With a few corrections, I think it will be a good improvement to the plugin.

In addition to inline comments, I think the filename needs to be changed. It is not used for the file created from the template, but it is used in IntelliJ UI so needs to be more descriptive. I'm not sure which would be the best, here are some ideas Stapler View.jelly.ft or maybe "Stapler Jelly View.jelly.ft` since there can also be Groovy views.

@timja can you give your two cents? Is this template a good starting point for Jelly Views? What would be a good name for it?

src/main/resources/fileTemplates/config.jelly.ft Outdated Show resolved Hide resolved
src/main/resources/fileTemplates/config.jelly.ft Outdated Show resolved Hide resolved
@duemir duemir requested a review from timja October 5, 2022 02:59
@duemir duemir linked an issue Oct 5, 2022 that may be closed by this pull request
@duemir duemir added this to the Hacktoberfest2022 milestone Oct 5, 2022
src/main/resources/fileTemplates/config.jelly.ft Outdated Show resolved Hide resolved

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" >
<f:section title="${%Put Section Name Here}">
Copy link
Member

Choose a reason for hiding this comment

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

why are we adding a section by default, lots of config pages won't need that, e.g. if they are included in another one

Copy link
Member

Choose a reason for hiding this comment

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

The guide suggests creating a template based on a file and then exporting the result and adding to a plugin. I'm guessing this template is based on config view with a section.

Are there multiple type of views each with different boilerplate?

What's the current process for creating a view? What would ideal one look like?

Copy link
Member

Choose a reason for hiding this comment

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

just an empty jelly file I think with the form and layout namespaces imported as they are the most common ones

Copy link
Contributor Author

@julieheard julieheard Oct 5, 2022

Choose a reason for hiding this comment

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

This template is based on config view with a section. Is the plan to have several different templates? In which case we can have templates with sections and templates without any sections.

Copy link
Member

Choose a reason for hiding this comment

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

based off of https://plugins.jetbrains.com/docs/intellij/providing-file-templates.html#file-templates-categories I don't think it should include section, just an empty jelly file should be fine

Copy link
Member

Choose a reason for hiding this comment

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

Well... /lib/form namespace is a Jenkins thing. Jelly is not popular but still has some use elsewhere.
ServiceNow Jelly tags < ServiceNow seems to be using it. Hence, I suggested an extra qualifier in the name. Maybe it should be "Jenkins Jelly View", not Stapler.

Maybe I am overthinking. ServiceNow seems to have a built-in editor, so it might be unlikely that any SNOW developer will discover this plugin because IntelliJ says that it supports *.jelly extension. Even if they do, they'll probably see the plugin name and avoid installing it anyway. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

Copy link
Member

Choose a reason for hiding this comment

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

OK. What do we agree on?

  1. Remove f:section tag
  2. Rename the file to Jenkins Jelly view.jelly.ft

Pretty sure we agreed on 1. Did I get the 2 right?

Copy link
Member

Choose a reason for hiding this comment

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

@julieheard please do the above two actions and I will merge the PR. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both changes done

julieheard and others added 3 commits October 5, 2022 12:28
Ok, agreed

Co-authored-by: Denys Digtiar <duemir@gmail.com>
Looks good to me

Co-authored-by: Denys Digtiar <duemir@gmail.com>
removed help link
@duemir duemir changed the title added a resources/fileTemplates folder and made a jelly config template file A file template is added for a Stapler Jelly View Oct 6, 2022
@julieheard julieheard requested a review from duemir October 10, 2022 07:48
@duemir duemir merged commit bff8b8a into jenkinsci:master Oct 11, 2022
@duemir duemir changed the title A file template is added for a Stapler Jelly View A file template is added for a Jenkins Jelly View Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contribute Jelly File Template
3 participants