You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org> on 2023/04/21 04:44:50 UTC
[GitHub] [camel-website] PasunuriSrinidhi opened a new pull request, #995: Updated rules.js
PasunuriSrinidhi opened a new pull request, #995:
URL: https://github.com/apache/camel-website/pull/995
1. Refactored the HtmlTitle rule to extract the title validation into a separate function validateTitle for better organization and reusability.
2. Used a for loop instead of forEach for better performance in the RelativeLinks rule.
3. Removed the start variable in the StructuredData rule and instead used the textContent property of the closing script tag to extract the JSON-LD content.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updated rules.js [camel-website]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus closed pull request #995: Updated rules.js
URL: https://github.com/apache/camel-website/pull/995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on a diff in pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174318296
##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
setup() {
this.on("dom:ready", event => {
const anchors = event.document.getElementsByTagName('a');
- if (anchors === null) {
- return;
- }
- anchors.forEach(a => {
- const href = a.getAttribute("href");
- if (href && href.value.startsWith("https://camel.apache.org")) {
- this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+ if (anchors !== null) {
+ for (let i = 0; i < anchors.length; i++) {
+ const href = anchors[i].getAttribute("href");
+ if (href && href.startsWith("https://camel.apache.org")) {
Review Comment:
Thanks for your help. I will add the href.value and modify the code accordingly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1527008397
@zregvart , Thanks for your response. I will into all the comments and I will modify according to it .
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updated rules.js [camel-website]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1997486605
closing old PR that was not active
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] zregvart commented on pull request #995: Updated rules.js
Posted by "zregvart (via GitHub)" <gi...@apache.org>.
zregvart commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1519611072
@PasunuriSrinidhi thanks, this still needs a bit of work, see my other comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1518547190
Updated the whole code. Can you please review the code and give suggestions?
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] davsclaus commented on pull request #995: Updated rules.js
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1533444854
@PasunuriSrinidhi let us know when you are done with the changes and we with help from @zregvart can take a look again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] zregvart commented on a diff in pull request #995: Updated rules.js
Posted by "zregvart (via GitHub)" <gi...@apache.org>.
zregvart commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1173472140
##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
setup() {
this.on("dom:ready", event => {
const anchors = event.document.getElementsByTagName('a');
- if (anchors === null) {
- return;
- }
- anchors.forEach(a => {
- const href = a.getAttribute("href");
- if (href && href.value.startsWith("https://camel.apache.org")) {
- this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+ if (anchors !== null) {
Review Comment:
I prefer the non-nested variant, i.e.
```javascript
if (anchors === null) {
return;
}
```
This [blog post](https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html) articulates the reasons why.
##########
rules.js:
##########
@@ -6,26 +6,30 @@ class HtmlTitle extends Rule {
description: "Title of the page must be defined"
};
}
+ validateTitle(title) {
Review Comment:
If we're going with this approach, I'd prefer putting all the logic in the `validateTitle` function.
```javascript
validateTitle(event) {
//...
}
```
And below we would have:
```javascript
this.on("dom:ready", validateTitle)
```
##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
setup() {
this.on("dom:ready", event => {
const anchors = event.document.getElementsByTagName('a');
- if (anchors === null) {
- return;
- }
- anchors.forEach(a => {
- const href = a.getAttribute("href");
- if (href && href.value.startsWith("https://camel.apache.org")) {
- this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+ if (anchors !== null) {
+ for (let i = 0; i < anchors.length; i++) {
+ const href = anchors[i].getAttribute("href");
+ if (href && href.startsWith("https://camel.apache.org")) {
Review Comment:
Missing `href.value` here
```suggestion
if (href && href.value.startsWith("https://camel.apache.org")) {
```
##########
rules.js:
##########
@@ -56,21 +59,12 @@ class StructuredData extends Rule {
};
}
- setup() {
- let start = -1;
-
- this.on('tag:open', event => {
- if (event.target.nodeName === 'script') {
- start = event.target.location.offset;
- }
- });
-
- this.on('tag:close', async event => {
- const tag = event.previous;
- if (event.target.nodeName === 'script' && tag.hasAttribute('type') && tag.getAttribute('type').value === 'application/ld+json') {
- const startIdx = data.indexOf('>', start) + 1;
- const endIdx = event.target.location.offset - 1; // omit the opening tag angled bracket
- const content = data.substring(startIdx, endIdx);
+
+setup() {
+ this.on('tag:close', event => {
+ const tag = event.target;
Review Comment:
For `tag:close` the event doesn't have a `target` property and that's why we need to remember the offset location of when the tag was opened. In the `tag:open` we also don't have access to attributes. This is why the logic here how it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on a diff in pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174317634
##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
setup() {
this.on("dom:ready", event => {
const anchors = event.document.getElementsByTagName('a');
- if (anchors === null) {
- return;
- }
- anchors.forEach(a => {
- const href = a.getAttribute("href");
- if (href && href.value.startsWith("https://camel.apache.org")) {
- this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+ if (anchors !== null) {
Review Comment:
Thanks for your suggestion .I will edit the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on a diff in pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174317970
##########
rules.js:
##########
@@ -6,26 +6,30 @@ class HtmlTitle extends Rule {
description: "Title of the page must be defined"
};
}
+ validateTitle(title) {
Review Comment:
Thanks for your review. I will write the entire logic in the validateTitle function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on PR #995:
URL: https://github.com/apache/camel-website/pull/995#issuecomment-1527008939
@zregvart , Thanks for your response. I will look into all the comments and I will modify the code accordingly.
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] PasunuriSrinidhi commented on a diff in pull request #995: Updated rules.js
Posted by "PasunuriSrinidhi (via GitHub)" <gi...@apache.org>.
PasunuriSrinidhi commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174318990
##########
rules.js:
##########
@@ -56,21 +59,12 @@ class StructuredData extends Rule {
};
}
- setup() {
- let start = -1;
-
- this.on('tag:open', event => {
- if (event.target.nodeName === 'script') {
- start = event.target.location.offset;
- }
- });
-
- this.on('tag:close', async event => {
- const tag = event.previous;
- if (event.target.nodeName === 'script' && tag.hasAttribute('type') && tag.getAttribute('type').value === 'application/ld+json') {
- const startIdx = data.indexOf('>', start) + 1;
- const endIdx = event.target.location.offset - 1; // omit the opening tag angled bracket
- const content = data.substring(startIdx, endIdx);
+
+setup() {
+ this.on('tag:close', event => {
+ const tag = event.target;
Review Comment:
Thank you so much for finding the errors. I will look into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] zregvart commented on a diff in pull request #995: Updated rules.js
Posted by "zregvart (via GitHub)" <gi...@apache.org>.
zregvart commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174943658
##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
setup() {
this.on("dom:ready", event => {
const anchors = event.document.getElementsByTagName('a');
- if (anchors === null) {
- return;
- }
- anchors.forEach(a => {
- const href = a.getAttribute("href");
- if (href && href.value.startsWith("https://camel.apache.org")) {
- this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+ if (anchors !== null) {
+ for (let i = 0; i < anchors.length; i++) {
+ const href = anchors[i].getAttribute("href");
+ if (href && href.startsWith("https://camel.apache.org")) {
Review Comment:
This was not changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-website] zregvart commented on a diff in pull request #995: Updated rules.js
Posted by "zregvart (via GitHub)" <gi...@apache.org>.
zregvart commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1174943445
##########
rules.js:
##########
@@ -6,26 +6,30 @@ class HtmlTitle extends Rule {
description: "Title of the page must be defined"
};
}
+ validateTitle(title) {
Review Comment:
I'm not seeing this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org