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

Change skin for mvn site report to use fluido #731

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

davewichers
Copy link
Contributor

The 4.0.0-Mx version of the -site plugin is broken if you don't specify a recent version of a skin it supports. So I am upgrading that plugin, specifying the skin version in the pom, and also created a custom site.xml based on AntiSamy's that references that skin. I also turned off the tag reports since they are all broken currently.

@kwwall
Copy link
Contributor

kwwall commented Jul 27, 2022

'target/site/property-updates-report.html' is still broken (missing trailing '}'. And not really sure that I like the image at the top of the site reports. Mostly just takes up room. I had already had updated to '-M3' version of the plugin but hadn't pushed yet and if it doesn't work without a skin, I obviously hadn't run 'mvn site', but I don't see any place where the '-M2' version was failing that '-M3' also isn't still failing. (E.g., the tag library documentation, which I what I think we miss most still isn't working as you mentioned in a private email.)

Anyway, if you create a GitHub issue and describe what reports this actually broken with it (so I can reference it in the next release notes), I will merge this.

BTW, you would think they would have had required the site.xml file to go under 'src/main/assembly' or maybe under 'src/main/resources' rather than 'src/site'. Adding more directories for no good reason (versus using an existing one) just means it's yet another place to forget about things. Not your fault, I know, but 'src/main/assembly' would have been better IMO.

@kwwall
Copy link
Contributor

kwwall commented Jul 27, 2022

Oh, one more thing. if we're going to have a logo on the 'mvn site' reports, I'd prefer to use the ESAPI logo instead (attached), scaled down of course.
esapi-logo

@davewichers
Copy link
Contributor Author

davewichers commented Jul 27, 2022 via email

@kwwall
Copy link
Contributor

kwwall commented Jul 27, 2022

Please create a GitHub issue, assign it to yourself, and then mention this PR in it and I'll do the merge. I just don't like to have commits / PRs not associated with GitHub issues (except for minor bookkeeping issues during releases, like removing '-SNAPSHOT', etc.) because I document the GitHub issues that were closed in release notes, not the specific PRs or commits and IMO, that's a more useful synopsis than just (say) looking at a change log.

@kwwall
Copy link
Contributor

kwwall commented Aug 17, 2022

On a previous episode, @davewichers wrote:

Tag pages were broken or blank before if I recall. I see no reason not to merge.

These tag pages are still broken. The only difference I see is that the indexes for:

  • Tag reference
  • Taglibdoc documentation
  • Tag library validation

no longer appear in the sidebar navigation panel so you just don't notice. I will write up a GitHub issue, but aside from some minuscule issues such as a messed up copyright notice (the year appeared as "Copyright © ${currentYear} ..."), I really don't see what was fixed. So to me this PR just looks like it changes the look-and-feel (admittedly, to one that appears a bit slicker) but really doesn't fix anything. So I need to know what to put into the the GitHub issue that I will create. So, I don't even know if this would be considered primarily a 'bug' or an 'enhancement' and thus I need some help in knowing what to put into the GH issue.

Also, note that with this PR merged, there will also a be downside of 'mvn site' no longer generating that sidebar navigation index which means that I had to create GitHub issue #733 so I wouldn't forget about the tag documentation not working. (Out-of-sight, out-of-mind.)

@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
Copy link
Contributor

Choose a reason for hiding this comment

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

Question for you @davewichers - This is just an XML namespace and often IRL, these URLs do not even exist and would return a 404. So if we were to change them to https would the schema validation still work or is there a chance it could break something. TBH, I've never seen https:// schema used in XML namespace and I'd be concerned if someone wrote a parser to do schema validations of Maven pom files (like might be in some IDEs), that this change could break something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@davewichers - I'm going to reject this particular change as it causes an error / warning in Eclipse and perhaps other zIDEs as well. See attached pic for example
ESAPI-pom-Eclipse
.

Copy link
Contributor

@kwwall kwwall left a comment

Choose a reason for hiding this comment

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

I have created a GH issue for this (issue #734), but you have to promise me to review it for accuracy before I merge this PR.

Also, please undo the changes to the XML namespace in the pom.xml (see previous comment.) Thanks.

@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
Copy link
Contributor

Choose a reason for hiding this comment

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

@davewichers - I'm going to reject this particular change as it causes an error / warning in Eclipse and perhaps other zIDEs as well. See attached pic for example
ESAPI-pom-Eclipse
.

Copy link
Contributor

@kwwall kwwall 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 to me.

@kwwall kwwall merged commit d605ecc into ESAPI:develop Aug 22, 2022
@davewichers
Copy link
Contributor Author

davewichers commented Oct 11, 2022 via email

@kwwall
Copy link
Contributor

kwwall commented Oct 11, 2022

@davewichers wrote:

I don't have time/interest in creating an issue. Please do so yourself so you can then reference it in your acceptance of this commit. I already spent an hour+ on this.

-Dave

Huh? I already created the issue for this (#734, no closed) and already merged this PR. Looks like you are maybe responding to old emails from your inbox??? Anyhow, this is all taken care of.

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.

2 participants