Skip to content

Conversation

Marcono1234
Copy link
Contributor

Where possible update Java documentation links to Java 11.
Additionally update some other links to use HTTPS (see also #5163).

Notable additional changes:

  • Only upgraded guides to Java 8 because apparently a lot of the guides were not migrated to newer versions
  • Only upgraded Java-EE links to Java 7 because newer EE versions are not maintained by OpenJDK anymore
  • Updated Eclipse IDE links to 2020-12; use the link variant which does not show a navigation sidebar
  • Consistently used "Java API Specification" when referring to https://docs.oracle.com/en/java/javase/11/docs/api/index.html
    • Removed Java version numbers because that would have to be manually changed again when links are upgraded
  • Replaced https://www.securecoding.cert.org with the https://wiki.sei.cmu.edu redirect

Comment on lines -45 to -48
<!-- LINKS NO LONGER VALID:
<p>Sun Bug Report: <a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6178997">Doc: it's not explicitly documented java.util.Calendar serialization are thread-unsafe</a></p>
<p>Sun Bug Report: <a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6231579">ArrayIndexOutOfBoundsException in BaseCalendar</a></p>
-->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the links (even though the bug repots can still be found) because the bug reports are about bugs in the JDK source code, which is likely not relevant to most of the users.

for different locales. For example, if the default locale is ENGLISH, the function <code>toLowerCase()</code>
converts a capital <code>I</code> to <code>i</code>; if the default locale is TURKISH, the function
<code>toLowerCase()</code> converts a capital <code>I</code> to the Unicode Character "Latin small
letter dotless i" (U+0131) (<a href="http://character-code.com/turkish-html-codes.php">Turkish HTML
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this link because it does not work anymore, is also probably not needed.

Comment on lines -48 to -49
<li>
Java Language Specification, 3rd ed: <a href="https://docs.oracle.com/javase/specs/jls/se6/html/conversions.html#5.4">5.4 String Conversion</a>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this link because it is about String concatenation in genereal and not related to toString().

Comment on lines -36 to +37
<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.5.3">Checked Casts at Run Time</a>,
<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.5.1">Reference Type Casting</a>,
<a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.10.3">Subtyping among Array Types</a>.
Java Language Specification:
<a href="https://docs.oracle.com/javase/specs/jls/se11/html/jls-5.html#jls-5.1.6">Narrowing Reference Conversion</a>,
<a href="https://docs.oracle.com/javase/specs/jls/se11/html/jls-4.html#jls-4.10.3">Subtyping among Array Types</a>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These §5.5 sections do not exist anymore in the latest versions, it looks like the content of that section has changed a lot.

<li>
Java API Documentation:
<a href="http://docs.oracle.com/javase/6/docs/api/java/lang/Comparable.html#compareTo%28T%29">Comparable.compareTo</a>,
<a href="http://java.sun.com/javase/6/docs/api/java/lang/Comparable.html">Comparable</a>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this link to Comparable because there is already a link to Comparable#compareTo.

* Holds if the string is a standard system property as defined in:
*
* http://docs.oracle.com/javase/7/docs/api/java/lang/System.html#getProperties()
* https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#getProperties()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not update this link to Java 11 because the System properties listed for that version might be different than the ones for Java 7.

aschackmull
aschackmull previously approved these changes Feb 23, 2021
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

This is awesome work, thank you very much! Changes LGTM, but pinging docs for a second look @github/docs-content-codeql

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Feb 23, 2021
@felicitymay
Copy link
Contributor

@aschackmull - do you have any thoughts about the failing tests? It looks as is the failure may be caused by one of the experimental queries (UnsafeTlsVersion.ql),but I can't see where this error is coming from:

A fatal error occurred: /home/runner/work/semmle-code/semmle-code/ql/java/ql/src/Advisory/Types/Generics_Common.ql does not exist.

I'm also not sure why we see a Java test failure for JdkInternalAccess.qlref.

In general the docs changes look great 😄
Once we've sorted out these test failures, it might be worth doing testing the changes on staging site for the CodeQL microsite, but otherwise happy for you to merge.

@aschackmull
Copy link
Contributor

The qhelp failure is due to the CI checking each .qhelp file as if it was a complete query help. In some cases we share snippets of qhelp between different queries, which are then imported into those queries' qhelp files. We used to name both complete qhelp and qhelp snippets with the same extension, but we've started to use .qhelp.inc for such snippets instead, so Generics_Common.qhelp should be renamed to Generics_Common.qhelp.inc for the test to pass.

The other failure is an internal test case that references the public query. This can't be fixed directly in this PR, so I'll handle that separately.

@aschackmull
Copy link
Contributor

@Marcono1234 Could you rename the Generics_Common.qhelp file to Generics_Common.qhelp.inc and update the references to it? (There are a couple in the other qhelp files in the same dir.) I believe that should fix the qhelp preview check.

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Feb 23, 2021

I have also changed the extension of other help files for which no query file exists, I hope that is alright.
Thought wouldn't it be better to use .inc.qhelp (and adjust CI) so IDEs / editors can still detect the file type and provide syntax highlighting?

@aschackmull
Copy link
Contributor

I have also changed the extension of other help files for which no query file exists, I hope that is alright.

That's excellent, thanks!

Thought wouldn't it be better to use .inc.qhelp (and adjust CI) so IDEs / editors can still detect the file type and provide syntax highlighting?

Probably yes. But that's a separate issue.

@aschackmull
Copy link
Contributor

Actually, I'll try to fix up our CI such that we can use .inc.qhelp instead - your suggestion gained some quick support.

@aschackmull
Copy link
Contributor

The CI has been updated to support .inc.qhelp as per your suggestion. Could you rebase to latest main (to include the fixed workflow) and rename the extensions to .inc.qhelp instead? Also, there are a number of the renamed qhelp files that are shared between multiple languages (sharing is done by copying the files and having a script verify that they're kept in sync), so that check currently fails; see config/identical-files.json and config/sync-files.py. The script is normally used to easily sync changes between the copies, but it doesn't support renames, so the copies and their references in the json doc needs to be renamed as well.

Where possible update Java documentation links to Java 11.
Additionally update some other links to use HTTPS.
@Marcono1234 Marcono1234 force-pushed the marcono1234/update-java-documentation-links branch from 070bca6 to 70d8e48 Compare February 25, 2021 23:44
@Marcono1234 Marcono1234 requested review from a team as code owners February 25, 2021 23:44
@Marcono1234 Marcono1234 force-pushed the marcono1234/update-java-documentation-links branch from 70d8e48 to 4513785 Compare February 26, 2021 00:13
@Marcono1234
Copy link
Contributor Author

Have rebased onto main, adjusted the extensions to .inc.qhelp and synchronized the necessary changes to the other languages.
For .qhelp renaming for the other languages, I have created #5275.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

CPP changes LGTM.

<qhelp>
<include src="CommentedOutCodeQuery.qhelp" />
<include src="CommentedOutCodeQuery.inc.qhelp" />
<include src="CommentedOutCodeCommon.qhelp" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This one also needs a rename in this PR, since it's being changed.

Copy link
Contributor Author

@Marcono1234 Marcono1234 Feb 26, 2021

Choose a reason for hiding this comment

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

It needs a rename because 1) that file is changed and 2) there is no corresponding .ql file, right?
I actually had already updated this by accident (but forgot the file rename), then reverted it and moved it to #5275 🤦

Is there a built script I can run to notice these errors myself or will it only work through trial and error by you having to check the internal CI errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have amended this change to the last commit and force-pushed. There are no other changes so the diff only shows these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It needs a rename because 1) that file is changed and 2) there is no corresponding .ql file, right?

Correct.

Is there a built script I can run to notice these errors myself or will it only work through trial and error by you having to check the internal CI errors?

Unfortunately not. The bulk qhelp processing actually doesn't require these renames at the moment - it is only the CI that chokes on the the .qhelp-without-.ql situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's a similar situation for javascript/ql/src/Metrics/FLinesOfSimilarCodeCommon.qhelp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry that I missed that. I have force-pushed the changes and went through all changed files to make sure that I have not missed another case, but I did not find any.

@Marcono1234 Marcono1234 force-pushed the marcono1234/update-java-documentation-links branch 2 times, most recently from 6f70ac5 to 3d56566 Compare March 2, 2021 21:09
@aschackmull
Copy link
Contributor

  Error: 3-03 10:43:57] [ERROR] generate query-help> /home/runner/work/semmle-code/semmle-code/ql/python/ql/src/Lexical/CommentedOutCode.qhelp: Could not find include file CommentedOutCodeExample.inc.qhelp
  Error: 3-03 10:43:57] [ERROR] generate query-help> /home/runner/work/semmle-code/semmle-code/ql/python/ql/src/Lexical/FCommentedOutCode.qhelp: Could not find include file CommentedOutCodeExample.inc.qhelp

@Marcono1234 Marcono1234 force-pushed the marcono1234/update-java-documentation-links branch from 3d56566 to b9c0193 Compare March 3, 2021 14:39
@aschackmull aschackmull mentioned this pull request Mar 4, 2021
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Everything looks good now, thanks! I'll merge this indirectly to handle the failing test, so as long as you don't change the head of this PR anymore it should show up as merged once I get that sorted.

@felicitymay felicitymay removed the request for review from jf205 March 4, 2021 11:24
@smowton smowton changed the title Update Java documentation links to Java 11 Update Java documentation links to Java 11; use .inc infix for included qhelp files Mar 4, 2021
aschackmull added a commit that referenced this pull request Mar 4, 2021
@aschackmull aschackmull merged commit 45f5228 into github:main Mar 4, 2021
@RasmusWL
Copy link
Member

RasmusWL commented Mar 4, 2021

Thanks @Marcono1234 👍 really awesome of you to do this 💪

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.

5 participants