jw3 commented on a change in pull request #26:
URL: https://github.com/apache/daffodil-vscode/pull/26#discussion_r728002336
##########
File path: .gitignore
##########
@@ -11,6 +11,10 @@ node_modules
target
.bsp
+# npm/yarn install files
Review comment:
We want to lock the yarn deps, so you can remove that ignore below.
Should update this comment to indicate that we only use yarn for package
management, so the npm lock is ignored here.
The npm lock shouldn't be generated given our dev docs, but just in case
someone does we will ignore it.
##########
File path: src/hexView.ts
##########
@@ -120,6 +121,19 @@ export class DebuggerHexView {
this.onDisplayHex(e.session, e.body)
break
}
+
+ let hexFileOpened = false
+
+ for (var i = 0; i < vscode.window.visibleTextEditors.length; i++) {
Review comment:
Prefer a for-of loop here
##########
File path: yarn.lock
##########
@@ -0,0 +1,4249 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Review comment:
Yarn is an alternative package manager to npm, but behaves similarly.
This file specifies the versions it resolved to perform the build, including
both our explicitly declared versions and all transients. If using npm this
file would be named package-lock.json and would have the same type of
dependency version info.
If this file is not present at the next build yarn will generate it by
resolving all dependencies again. Because libraries can and do specify
floating versions, the only way to achieve a repeatable build is to commit a
lock.
Rule of thumb is that applications get a lock file and libraries do not.
This extension would be considered an application here.
For comparison, Scala doesn't have this problem at all since you must
explicitly declare versions for dependencies, and that requirement goes all the
way down through all your transients, providing stable versions by default.
##########
File path: yarn.lock
##########
@@ -0,0 +1,4249 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Review comment:
How do we add license header for autogenerated files?
##########
File path: yarn.lock
##########
@@ -0,0 +1,4249 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Review comment:
I would have to look for best practices, but offhand I would say you
don't have to regen the entire file, can use the cli and or your explicit
dependency list to target a specific package to update.
So if given a finding in a dependency you could surgically update it and
only get changes to that and whatever transients it brings along. That would
be the most stable way to do it.
As with any dependency management, YMMV.
##########
File path: yarn.lock
##########
@@ -0,0 +1,4249 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Review comment:
> Looks like there's hundreds of dependencies if you include the
transitivies.
Well you would probably only update a top level dependency, not a transient,
otherwise you might create undefined behavior the whole way down by changing
someone else's dep.
> There's an easier way - let dependabot do some of the work.
Yes, concur.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
