You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/05/21 12:35:05 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #2720: Configure opportunistic formatting of JavaScript
ctubbsii opened a new pull request, #2720:
URL: https://github.com/apache/accumulo/pull/2720
As part of the formatting profile during the Maven build, attempt to
format JavaScript files in the monitor if the `npx` command is available
to run js-beautify
Include formatting changes to the monitor's JavaScript files
This fixes #2713
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135217262
Should work for you now.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135822629
> Should work for you now.
Yup works great!
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135193868
It looks like the problem might be with npx command-line parsing for multiple files using a wildcard. After troubleshooting, it looked like the main difference was the version of npx that @milleruntime was using (3.5.2?, whereas I tested with 8.3.1).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135119685
I downloaded your branch, checked out a JS file from main so there would be something to format and then ran my usual `mvn clean verify -DskipTests -Dspotbugs.skip`. It looks like it is detecting the `npx` binary but when it runs I am getting the error:
<pre>
[INFO] --- exec-maven-plugin:3.0.0:exec (format-javascript) @ accumulo-monitor ---
{ Error: EEXIST: file already exists, mkdir '/home/mpmill4/workspace/accumulo/server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js'
at Object.fs.mkdirSync (fs.js:885:18)
at writePretty (/usr/local/lib/node_modules/js-beautify/js/lib/cli.js:487:12)
at makePretty (/usr/local/lib/node_modules/js-beautify/js/lib/cli.js:473:9)
at Object.processInputSync (/usr/local/lib/node_modules/js-beautify/js/lib/cli.js:465:9)
at Array.forEach (<anonymous>)
at Object.exports.interpret (/usr/local/lib/node_modules/js-beautify/js/lib/cli.js:313:19)
at Object.<anonymous> (/usr/local/lib/node_modules/js-beautify/js/bin/js-beautify.js:4:5)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
errno: -17,
code: 'EEXIST',
syscall: 'mkdir',
path: '/home/mpmill4/workspace/accumulo/server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js' }
[ERROR] Command execution failed.
org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1)
</pre>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r879547571
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
We could potentially add `-w, --wrap-line-length Wrap lines that exceed N characters [0]`. I think things look fine as is but I wanted to point it out since we do have a line wrap set for the java 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r879408607
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
I can test this out locally.
Will this change be able to run on any Github Action or Jenkins jobs?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135129986
> I just made a comment about the error I am seeing but it looks like you already merged.
Yeah, I just saw that. I can revert if necessary, but I'd like to try to figure out your error. What version of js-beautify do you have installed?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r878691793
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
I'd appreciate any feedback on whether we should be using any additional formatting options on the command line here.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#issuecomment-1135202369
Since it works with individual files on the version @milleruntime has, I'll modify the script to try that, if the first pass fails. I would just always do individual files, but it is really slow.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r879812344
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
> We could potentially add `-w, --wrap-line-length Wrap lines that exceed N characters [0]`. I think things look fine as is but I wanted to point it out since we do have a line wrap set for the java code.
I looked into this, but it wrapped some things very oddly. I think it'd be better to just wrap manually, if necessary.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r879717523
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
> Will this change be able to run on any Github Action or Jenkins jobs?
I believe npx should be installed on the Ubuntu images during GitHub Actions, but it won't run because I defined it in a profile, and we have `-DskipFormat` set on GitHub Actions tasks. I did not set up a corresponding verifyFormat profile, since that would be a bit more complicated, and I didn't want to assume developers had `npx` available (or had a particular version of js-beautify installed).
It might run in Jenkins, I'm not sure. But nothing will be checked in.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2720:
URL: https://github.com/apache/accumulo/pull/2720#discussion_r879727177
##########
server/monitor/src/main/scripts/format-js.sh:
##########
@@ -0,0 +1,27 @@
+#! /usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+# This script will attempt to format JavaScript files
+
+if hash npx 2>/dev/null; then
+ npx js-beautify -r -n -j -s 2 src/main/resources/org/apache/accumulo/monitor/resources/js/*.js
Review Comment:
OK. FYI I had to install `npm` and then setup `npx` module to get it to work on my Ubuntu 18
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #2720: Configure opportunistic formatting of JavaScript
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2720:
URL: https://github.com/apache/accumulo/pull/2720
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org