Make MarshalledEntry and MarshalledEntryFactory part of the persistence SPI

https://issues.jboss.org/browse/ISPN-9693

@danberindei @wburns This is just from my marshalling PR to reduce the diff size https://github.com/infinispan/infinispan/pull/6240

@wburns @danberindei I was thinking about this over the weekend ... Maybe now is a good time to also fix the fact that we expose InternalMetadata via MarshalledEntry. Our store implementations rely on a lot of the methods that are only exposed in InternalMetadata, and I'm sure they would be useful for other store impls. So I think this is mainly a naming issue.
Proposal:
Create org.infinispan.persistence.spi.EntryMetadata for the SPI:
interface `EntryMetadata extends InternalMetadata {}`

Update org.infinispan.persistence.spi.MarshalledEntry so that InternalMetadata getMetadata() is removed and EntryMetadata getEntryMetadata is added.
For backwards compatibility, we must then add InternalMetadata getMetadata() to the deprecated org.infinispan.marshall.core.MarshalledEntry

Sounds good to me, however I don't think the new interface should extend InternalMetadata. It should be the inverse and we hide InternalMetadata if we need to have some methods that are for internal use only. It might be that we don't need to hide anything, but I would rather have it available if we need to.
interface EntryMetadata {
 ... needed methods ...
}

interface InternalMetadata extends EntryMetadata {
 ... methods just for us ...
}

This would mirror how we handle DataContainer and InternalDataContainer.

However, I am fine handling that in another PR so we can integrate this one right now :)

@wburns Yeah that sounds a better approach and I agree lets put it in another PR :smile:

@ryanemerson I think the reason for the name is that the timestamps are stored in the InternalCacheEntry implementations, not in the Metadata implementations. We allow custom Metadata implementations, and we don't require them to store the timestamps, so the timestamps are part of the persistence SPI but not part of the custom metadata SPI.
Personally I'd like to keep all the timestamps and the lifespan/maxIdle parameters out of Metadata and out of InternalMetadata, so they wouldn't be included in getMetadataBytes() and instead each store could use its own custom storage. I'm thinking especially of JDBC stores, but for other stores as well it may be nice to have the expiration time at a fixed offset in the entry. Not sure how to do it in a backwards compatible way, though :)

@ryanemerson @wburns I just realized that Metadata doesn't have isExpired(), so it should be safe to keep everything as it is now only adding adding ByteBuffer getUserMetadataBytes() to MarshalledEntry for stores that can/need to store the timestamps separately and moving the InternalMetadata methods to MarshalledEntry (and keeping the old methods as deprecated). That way Metadata would only hold data that needs to be updated atomically on a put(), and we could get rid of the Builder :)
Somewhat related, we should really move most of the methods in CacheEntry to a new interface ContextCacheEntry, they really don't make sense when returned from getCacheEntry().

@danberindei Just to make sure I follow before doing anything. You're proposing that the InternalMetadata methods should be directly part of the new persistence.spi.MarshalledEntry interface like so:
   ByteBuffer getKeyBytes();

   ByteBuffer getValueBytes();

   K getKey();

   V getValue();

   // Start of new methods
   Metadata getUserMetadata();

   ByteBuffer getUserMetadataBytes();

   long created();

   long lastUsed();

   boolean isExpired(long now);

   long expiryTime();


newLayer + overlayImage

I have try in many way, but without success.
I wanna add a overwatch to the image i'm rendering, with the code bellow

But when render try to load the overlay returns an error,

I'm using browser,

Try to load the overlay.png with camanJS and use camanJS instance for overlayImage() function.
Update: You can also load with new Image(), and use an Image instance for overlayImage.


file download fail

$ vcpkg z-ce update arm
warning: vcpkg-artifacts is experimental and may change at any time.
Updating registry data from https://artifactory.keil.arm.com/artifactory/vcpkg-ce-registry/registry.zip
warning: failed to download from https://artifactory.keil.arm.com/artifactory/vcpkg-ce-registry/registry.zip
error: Error: failed to download /opt/vcpkg/downloads/14446159-registry.zip from any source

Thanks a lot for reporting. I have just corrected the wrong url. Please update the vcpkg-configuration.json file and let me know whether this fixes your problem.

@Johnmc104, is this issue solved? Could you please close this issue to confirm? Thanks.


Firebase Auth Handle is not working

I'm using the below URL to redirect but this isn't working.  Giving me error
'Unable to process the request due to missing initial state. This may happen if browser sessionStorage is inaccessible or accidentally cleared.'
https://.firebaseapp.com/__/auth/handler

I am using this directly from Firebase and have no issues
TwitterAuthProvider twitterProvider = TwitterAuthProvider();         userCredential = await _auth.signInWithProvider(twitterProvider);

@arslanrazacrew
Sorry it took so long.
This plugin launches a browser, but it is not running on xxx.firebaseapp.com domain, so there is no session where the xxx.firebaseapp.com page is accessible.
I have verified this in my environment and it does not work with xxx.firebaseapp.com/__/auth/handler.
If the firebase domain is available, it works if you configure it to redirect to another page instead of xxx.firebaseapp.com.


feat(windows): group ip addresses to interfaces based on interface index

On Linux, all ip addresses of each interface are stored in a single NetworkInterface structure. On windows each IP address was stored in a distinct NetworkInterface structure. With this change, windows output structure is now the same as on Linux - one NetworkInterface structure for each network interface.

Hi @scanban, I think this will break current Windows users impls right?

Hi @scanban, I think this will break current Windows users impls right?

depends, the only problem for them is that now each NetworkInterface have multiple addresses.
But this is exactly how it works on linux. So cross platform code should work as it is.

Hi @scanban, I think this will break current Windows users impls right?

depends, the only problem for them is that now each NetworkInterface have multiple addresses.
But this is exactly how it works on linux. So cross platform code should work as it is.

Makes sense. Thanks for sharing.

I think this will break current Windows users impls right?

I think this is a fix more than a feature.
Intuitively, multiple addresses of the same interface should not be considered as multiple NetworkInterface.


Add support for user namespaces in stateless pods (KEP-127)

Hi!
This PR implements support for user namespace in Kubernetes pods. I'm opening the PR to see if you think this approach is sound, I'll add tests later, of course, but don't need to wait on that to see if this direction makes sense.
The 3 subitems listed in #7063 are solved in some way here, but not all possible solutions are implemented. In particular:

The netns problem: it is solved in the first commit (Let OCI runtime create netns when userns is used) and, as requested by Mike Brown in a community meeting, I left intact the code-path that are used today.
Ownership of the rootfs: three possible solutions are listed there. I chose just to take the commits from PR #7310, that uses a chown and adds the notion of capabilities, to say if some snapshotter supports idmap mounts (support for idmap not implemented). I've squashed the commits in that PR, rebased, fixed conflicts and just improve some code-paths based on my PoC patches. We opened a different PR for those changes first as we thought it may make sense to merge it before, but now that this PR is open, I think we can superseede that PR.
Just implement userns support: this is done in commit "cri: Support pods with user namespaces"

The other solutions listed in the issue for the ownership of the rootfs are fuse and idmap mounts. I'm not particularly interested in fuse solutions, as that changes the stack a lot and we just prefer to use idmap mounts/chown. We can implement idmap mounts in the future with this capability commits included (this probably will need to be probed at runtime, as support depends on the kernel and fs being used. I haven't checked how sound these commits are regarding runtime discovery, those were written by @dgl). There is an earlier attempt to use idmap mounts for the rootfs too: https://github.com/containerd/containerd/pull/5890, I haven't looked into merging that with the capabilities approach.
While it is not needed, it will be nice to also merge #7141 for 1.7 too, as that reduced the storage overhead when using userns (we only create inodes and hardlink instead of a full blown copy of the rootfs for each pod). Probably it reduces the pod startup latency too :)
As a side note, I noted there is pkg/cri/server and pkg/cri/sbserver that seems to be a c&p with some small changes. I've only implemented support for userns in pkg/cri/server but it is trivial to duplicate the code in sbserver. See commit "WIP: Fix sbserver compilation". I wasn't sure if that is desirable or not. Just let me know :)
To review, consider reviewing commit by commit. Each commit is atomic and has clear explanations.
To test this, a k8s 1.25+ cluster and the field "hostUsers: false" in the pod.spec should be enough. Feel free to reach out to me on slack if there hit any issue to test this.
I'll try to join the next community meeting in case someone wants to discuss this in more detail.
I was planning to add unit tests, but if you have any suggestion or thoughts on this, don't hesitate to let me know :)
Fixes: #7063
cc @rodnymolina @dgl

Thanks for working on this! I will definitely take some time to look at this soon. I've added the 1.7 milestone as we clearly want to have support for the new CRI API changes for userns KEP in containerd in our next major release.

We are working on a feature that depends on user namespaces, so we are very interested in your patch.
I downloaded and try your patch , but I haven’t been able to enable namespaces. When I follow the instructions here (https://kubernetes.io/docs/tasks/configure-pod-container/user-namespaces/) and add hostUsers: false key, I still get no user namespace created. In fact, if I check the bundle that containerd creates, there is no reference to user namespaces whatsoever.
What are we doing wrong?

@asierHuawei That is good to know, thanks! Are you sure you enabled the feature gate on all k8s components (not just the kubelet)? If that doesn't do the trick, something else simple should be missing. Let's continue this on slack, I'm rata in k8s slack, Rodrigo Campos or rata in CNCF slack

As previously discussed with @rata, I was able to verify the proper behavior of this patch for unprivileged pods. I may add some feedback later on once I review the code changes in more details.

Could you add integration tests too?
https://github.com/containerd/containerd/tree/main/integration

@rata I was able to get it working enabling the feature gates. In our tests, everything seems to work. So far so good.

Most comments are solved now (squashed all to the commits introducing those changes). Left one question open and will solve the remaining one in the coming days
@dgl the remaining one is related to the rootfs remaping (https://github.com/containerd/containerd/pull/7679#discussion_r1043341342) if you want to tackle that, please go ahead. Otherwise I'll do it :)
Thanks again for the review time!

@fuweid @mikebrow: I've fixed all the open comments now. I think people already went over the code, is there something that needs discussion?
If there is anything else that needs discussion, would it be safe to remove such label and I add some tests? Or do you think some major change might still be requested?

@fuweid @mikebrow: I've fixed all the open comments now. I think you already went over the code, is there something else that needs discussion?
If there isn't anything else that needs discussion, would it be safe to remove such label and I add some tests? Or do you think some major change might still be requested?

SGTM to come out of draft and add tests.. I think you are definitely close enough!

@rata yeah. it is good to add test cases. Thanks!

@mikebrow @fuweid Cool! Should we remove the needs-discussion label, then?

@mikebrow @fuweid Cool! Should we remove the needs-discussion label, then?

needs/discussion is not the same here in containerd as it would be in k/k  .. I think we still need some discussion around support in windows? and will be interesting to check out the integration tests.. would be nice if we have some k/k e2e buckets to enable and cri-tools as well..  But I would also like to get this out in 1.7 if the timing is right!

@mikebrow

think we still need some discussion around support in windows?

I've already done this. Windows people told me (it was posted in the KEP review too by Mark) they won't support this, this is why also the CRI changes change the Linux-specific parts only.

and will be interesting to check out the integration tests.. would be nice if we have some k/k e2e buckets to enable and cri-tools as well.. But I would also like to get this out in 1.7 if the timing is right!

Yes, I'll add tests to this PR. But anything else is needed to discuss on this PR?

@mikebrow

think we still need some discussion around support in windows?

I've already done this. Windows people told me (it was posted in the KEP review too by Mark) they won't support this, this is why also the CRI changes change the Linux-specific parts only.

ok thx

and will be interesting to check out the integration tests.. would be nice if we have some k/k e2e buckets to enable and cri-tools as well.. But I would also like to get this out in 1.7 if the timing is right!

Yes, I'll add tests to this PR. But anything else is needed to discuss on this PR?

IMO, not for this PR. Moving status along from discussion to update for tests.. and review...

Added unit tests for userns parts. I need to check tests for the remapped snapshot (@dgl can you look into this?) and integration tests
I've added a TODO list in the PR description with these items.

/ok-to-test

I'm trying to add integration tests for this, but I'm running into issues.
I'm unsure why this is not working: integration/pod_userns_linux_test.go. I'm running locally with:

Start patched containerd as root with a custom root, state and grpc address to avoid clashes with the system containerd
As root running: CONTAINERD_ROOT=<proper path> CONTAINERD_STATE=<proper path> CONTAINERD_SOCK=<proper path> FOCUS=TestPodUserNS make cri-integration

If you see the test file (integration/pod_userns_linux_test.go), you can see I added a print to see the sandbox config, and the sandbox config seems to have all that is needed (the userns config) and adding prints on the server, I see that the code to create a userns is used. Also, if I connect via k8s to the same instance and create a pod, the pod runs with userns just fine.
This way of running is the info I could gather by looking into the Makefile, github actions and scripts around the repo. I might have missed some detail, but I didn't find any docs to confirm.
What am I missing?
Now I'm waiting on the CI to run this, to see what happens there. But it takes like a lot of time (around 20-40 mins?) so it is a PITA.

Yea, on github CI it also fails.
The print I added seems to say userns are used (https://github.com/containerd/containerd/actions/runs/3752785342/jobs/6375415190#step:18:908, search for "UserNamespace", it uses the pod mode and a mapping)
And the error is that /proc/selc/uid_map is not using a user namespace, but exposing just all the UID space (the big number means all UIDs). It should contain 65536, that is the length we specified. See the error here: https://github.com/containerd/containerd/actions/runs/3752785342/jobs/6375415190#step:18:916.
I guess I'm missing something obvious, but I can't see it. Any help is super welcome! :)

Ok, as we debugged with @fuweid the container config in the integration tests was missing the userns option. When k8s is the client connecting to the CRI is not an issue, as the kubelet sets the container config with the userns config. Adjusted the integration tests to do that too, and as discussed with @fuweid a follow-up PR might be created so the integration tests is aligned with what kubelet does.
tl;dr: Added integration tests now, please have a look!

Fixed so integration tests are skipped with sandbox runtimes, as userns there is not needed/supported. All should be green now

@AkihiroSuda thanks for the review, solved now! :)


[darwin-framework-tool][interactive] Add Ctrl+G as a shortcut key to …

…trigger a resubscription to the last used MTRDevice
Problem
This PR adds a shortcut to trigger resubscription in interactive mode (Ctrl+G).

Fast-tracking platform-specific fix with platform owner review.


Add bullets to the calendar events instead of background color

Hi, is there anyway we can add bullets beneath the date of the month instead of background color the signify that there is an event that date? If so, how would i do it because, i tried editing the monthview-primary-with-event css class but I was not able to see any changes. Any help would be appreciated.

Nvm, I figured it out. The code for bullet points beneath the date is
.monthview-primary-with-event:after {
content: '⬤';
display: block;
font-size: 5px;
font-weight:bold;
line-height: 0px;
color: black;
}


[Music] Fixes

Description
Fixes for #1734 and #1806
Checklist

[x] I read the extension guidelines
[x] I read the documentation about publishing
[x] I ran npm run build and tested this distribution build in Raycast
[x] I checked that files in the assets folder are used by the extension itself
[x] I checked that assets used by the README are placed outside of the metadata folder

Is it okay that I try something with the date @Rawnly :)

Is it okay that I try something with the date @Rawnly :)

Uh yeah I didn't get it!


Updated correct contact address

Description of changes
Updated to the correct contact address.
Issue Resolved

Fixes #505

Thank you, @mangosing!


search package on the website keeps loosing focus after I press a key

typiing into search is very tricky
https://github.com/conan-io/web/assets/11256077/81b65de7-9a0e-487b-84f7-a4263a2dfc70

I have the same issue on Firefox and Safari, but not on Chrome.

Hi @DenisYaroshevskiy @netheril96
I'm not able to reproduce the issue in any browser. I just checked it in Safari, Chrome, and Firefox. Could you refresh the web page (and the cache) and try again? We deployed a recent frontend version that solved some issues a few days ago. Now, whenever you load the conan.io/center/ site, the focus should be automatically put in the Conan search bar, and there is no need to use the mouse to start typing.
Apart from that, I opened a PR https://github.com/conan-io/web/pull/145 to avoid typing blank spaces, which are not allowed in any Conan recipe name.

Here is the recording today
https://github.com/conan-io/web/assets/836839/f6985abc-fa8b-4030-8b55-8b00ffae4282


Pop up view

Add pop up view with JavaScripts.
Solved linters errors.
Created pop-up.css file.
Created pop-up.js file.
Modified index.html and styles.css files to restructure web layout.

Hi Team,
While you put in a lot of hard work in this project, unfortunately, I cannot proceed with the review of your code.

Invalid Code Review Request 🚫

You have submitted a project that doesn't include all the necessary linters checks, yours is lacking WebHint and LightHouse 👇🏽

Make sure to update your .github/workflows/linters.yml with the appropriate one you can find here
Your Code Review Request will be marked as invalid in your Dashboard, but don't worry you can always try again! 💪
Cheers and Happy coding!👏👏👏
If you have any questions or need clarification on anything, feel free to leave a comment in the pull request thread and don't forget to tag me so I'll receive the notification.
Please note

To avoid confusion and delays, please do not open a new pull request for re-reviews. Use the same pull request submitted for the first review, unless otherwise instructed.

Invalid Code Review Request does not count towards the code reviews limit.

@didierganthier thank you for your feedback. I have followed your advice by updating the linter.yml file using the link you sent. However, the lighthouse linters is failing, and I do not know how to fix it. Your response on how to go about it will be appriciated, thanks.


docs: Add a documentation for the AntennaAttributionDocumentReporter

This PR adds a end-user documentation for the AntennaAttributionDocumentReporter.

Please add a link to this document in the reporter section of the main README.md.

I guess we could simply replace the link to https://github.com/eclipse/antenna in the Reporter section of README.md with a link to this document, as this document links to https://github.com/eclipse/antenna.

Also, the commit message should auto-close https://github.com/oss-review-toolkit/ort/issues/2674.

BTW, that I'm missing in general is documentation on how to create the PDFs themselves, no matter whether they are provided inside a template bundle JAR, or as individual files in a directory. IIUC, you need to add special markers (which get replaced) to those PDFs, and that should be explained, as well as a recommended way to create the PDFs, like using a single Word / Open Office / Libre Office document, and exporting the individual pages from it as the PDF templates. Does such a guide already exist, as part of Antenna maybe?

@sschuberth Unfortunately there is only the documentation in Antenna for creating the JAR file with existing PDFs. In my opinion, we should add this description for the creation of the PDFs in Antenna.

In my opinion, we should add this description for the creation of the PDFs in Antenna.

I agree. Could you please do that first and then come back to this PR to update it accordingly?


Provide a way to work with Allure CLI tool offline

Provide a way to work with Allure CLI tool offline - currently from what I understand it's downloading maven artifacts as part of it's process and that causes issues when working behind proxy with authentication or without internet connection. Actually I think CLI tool should not use maven at all - I would use allure-maven-plugin if I wanted to go maven path. I'm trying to use it for javascript project so dealing with maven seems not correct here.

@bartoszkaczorek thanks for feedback!
New command line tool works in offline mode. See http://wiki.qatools.ru/display/AL/Allure+CLI for more information

Great to hear that! Can you tell if Allure Jenkins plugin will be able to work offline as well? Had similar issues with it and that was my initial reason to use allure-cli instead.

Can you tell if Allure Jenkins plugin will be able to work offline as well?

It is almost done, see https://github.com/jenkinsci/allure-plugin/pull/23


signFromFile return null

I'm doing the example of the README.md and at the moment of executing the method signFromFile returns a null value and does not sign the xml
this my code:
use Greenter\XMLSecLibs\Sunat\SignedXml;
require 'vendor/autoload.php';
$xmlPath = '20516109549-01-F001-70.xml';
$certPath = 'certificate.pem'; // Convertir pfx to pem
$signer = new SignedXml();
$signer->setCertificateFromFile($certPath);
$xmlSigned = $signer->signFromFile($xmlPath);
var_dump($xmlSigned);

Fixed, thanks.

I'm still receiving null, xml attachment and code
signed.zip

You need to convert cert.pfx -> cert.pem, tutorial here
Use xmldsig v5.0.1 in composer.json

if it is converted and keeps returning null, I attach the arvhivo .pem
certificate.zip

sorry it was my mistake, I only had to update the package with composer but the tag X509SubjectName does not appear

X509SubjectName is not required. That is an additional information.
ds:X509Data/ds:X509Certificate contains that information.

Okay thanks for your help :D


[Enhancement] Import the starcachelib to block cache by staros to replace starcache submodule.

Fixes #issue
As the starcache library has been installed into staros output and imported into starrocks. So we remove the original starcache sumodule and depend on the starcache library directly.
What type of PR is this:

[ ] BugFix
[ ] Feature
[x] Enhancement
[ ] Refactor
[ ] UT
[ ] Doc
[ ] Tool

Checklist:

[x] I have added test cases for my bug fix or my new feature
[ ] This pr will affect users' behaviors
[ ] This pr needs user documentation (for new or modified features or behaviors)

[ ] I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

[x] I have checked the version labels which the pr will be auto-backported to the target branch

[ ] 3.0
[ ] 2.5
[ ] 2.4
[ ] 2.3

[FE PR Coverage Check]
:heart_eyes: pass : 0 / 0 (0%)

[FE PR Coverage Check]
:heart_eyes: pass : 0 / 0 (0%)

[FE PR Coverage Check]
:heart_eyes: pass : 0 / 0 (0%)

@mergify rebase main

[FE PR Coverage Check]
:heart_eyes: pass : 0 / 0 (0%)


feat(pointTool): The distance between limit points can't be less than 0.2px

Please extend this function to the PolygonTool and LineTool

Hi @Asliuyong   !
First of all, we want to express our gratitude for your significant PR in the OpenMMLab project. Your contribution is highly appreciated, and we are grateful for your efforts in helping improve this open-source project during your personal time. We believe that many developers will benefit from your PR.
We would also like to invite you to join our Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us ：https://discord.gg/UjgXkPWNqA
If you have WeChat account，welcome to join our community on WeChat. You can add our assistant ：openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends：）
Thank you again for your contribution❤

:tada: This PR is included in version 1.0.0 :tada:
The release is available on npm package (@latest dist-tag)
Your semantic-release bot :package::rocket:


proposal: Go 2: revisit explicit annotations in :=

Before Go 1 := would only work if every name on the lhs had not been declared in the current scope. This made it hard to write common code like
x, err := f()
//...
y, err := g()

Either y would require an explicit declaration with var or there would need to be an ugly workaround such as
y, err2 := g()
err = err2

Among other peccadilloes this meant copying a bit of valid code from one place to another could make it invalid and it would need to be rewritten.
This made := much less useful than it could be so the current behavior of implicitly reusing existing names defined in the current scope was introduced and erroring out if none of the names were new.
This made := much more useful, but has made it too easy to shadow variables in outer scopes.
When it was introduced, there was much discussion in the mailing list thread announcing the change (apologies but I cannot find a link) for a way to explicitly annotate the names to reuse such as
y, ^err := g() // mnemonic for "reusing variable defined above"

This was dismissed—if I recall correctly (again apologizes for not being able to track down the original thread)—because it would mean that copying a bit of valid code from one place to another could make it invalid, as annotations would need to be added or removed depending on the new context.
However, this is subtly true with the current behavior := depending on the names involved and what is or is not in the current scope, leading to bugs caused by accidental shadowing that are hard to track down due to the implicit nature of what is shadowed when. This is particularly vexing with named returns.
With explicit annotation it would still be possible to accidentally shadow a variable, but it would be easier to see what was happening at a glance and possibly easier to detect mechanically.
I think the idea of explicit annotation should be revisited for Go 2. I propose the following semantics

x, y, z := f() follows the pre-Go 1 semantics—all names must be unbound in the current scope
x, ^y, ^z := f() reuses y and z as defined in the current scope or an outer but non-global scope†
annotating a name that does not exist is an error
at least one name on the lhs must not be annotated

† making it incompatible with reusing variables in package global scope is a bit uneven but seems a wise safety precaution
This should be entirely go fix-able as the correct annotations to mirror the Go 1 semantics could be applied mechanically.

Dup of #377?

That thread covers a lot of potential changes to :=. It looks like the explicit annotation was brought up after a dozen or so posts and eventually dominates the conversation (I only skimmed). I don't mind this being closed as a dup but #377 is closed due to age.

Before Go 1 := would only work if every name on the lhs had not been
declared in the current scope. This made it hard to write common code like

x, err := f()
//...
y, err := g()
This is not correct,
https://play.golang.org/p/aD8_f2r8M4
On Fri, Aug 4, 2017 at 10:04 AM, Brad Fitzpatrick<EMAIL_ADDRESS>wrote:

Dup of #377 https://github.com/golang/go/issues/377?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/21303#issuecomment-320121591, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA9ajSza1u0_AHhT0L619SCUtrY4qks5sUmAjgaJpZM4OtIK3
.

@davecheney that's Go 1 code.

^ is the bitwise xor operator.
I'm not really sure this is that useful to be honest.
If I think something's going to be shadowed, I usually explicitly declare the variables.

Another alternative would be to annotate the new names instead of the old: that would make a ShortVarDecl equivalent to an Assignment.
For example, we could shift the colons from the = symbol to the new names:
x:, err: = f()
// …
y:, err = g()

I don't really care about the syntax. There are other suggestions in #377 and the mailing list thread linked in there. I'm sure the Go team will pick something reasonable and pleasant in the face of all other constraints, if they decide to implement.
I'll use ^ in any examples that I write for consistency with the first post. I know it's an operator already but I think the polysemy should be fine in this case since a ^ would already be illegal in any place where this would come up, but, if it's not, that's fine.
I just want to be able to explicitly and concisely reuse variables from outer scopes without having to entirely and awkwardly rewrite the line with multiple var declarations. I want it to be easier to spot and avoid shadowing.
Annotating new names rather than old names would make it easier to avoid accidental shadowing in the first place.
I was thinking that another option would be to keep the default rules of := as it is now, but allow annotations to mean "use the one in an outer scope".  I'm not sure how far the plan is to go with that since that would mean no new keywords. It would mean you'd only be able to annotate old names, though.
I'd prefer "you always need to annotate" as it would mean not seeing an annotation had as much meaning as seeing an annotation, even though that would mean having to annotate more often.

I don't really care about the syntax.

Then why not just use
x, err := f()
y, e2 := g(); err=e2

Before Go 1 := would only work if every name on the lhs had not been declared in the current scope.

I don't believe this is true, although I remember the discussion on changing the semantics of :=.
See my earlier comment: https://github.com/golang/go/issues/21161#issuecomment-320635780
From that previous discussion, ISTR favouring a syntax like this:
:a, :err = foo()     // equivalent to a, err := foo()
:b, err = foo()      // b is newly declared; err is not.

This doesn't avoid accidental shadowing though - I'm pretty sure you
don't want to ban all shadowed variable names.

@as I don't care about the specific syntax used as long as there is some syntax with the desired semantics
@rogpeppe I could easily be misremembering or confusing it with a comment from the Go team that "it worked like that but then we changed it". Maybe even another language entirely. I tried to find the thread to check before posting but could not so I crossed my fingers and posted. Oh well.
At any rate, no I don't want to ban shadowing.
Shadowing is good. But it can also be awkward and easy to shadow too much.
The proposal is for some way to keep the nice bits of := but add a way to explicitly allow reuse of a variable defined in an outer scope. The current behavior can cause subtle problems especially with named returns and common names like err so use of := requires a dose of paranoia.
If that's the :a, err = foo() syntax, great.

I would like it if shadowing an outer scope in the short declaration were banned.
So, for example, this would be legal:
func Example() (int, error) {
    x, err := thingOne()
    if err != nil {
        return 0, err
    }
    y, err := thingTwo()
    if err != nil {
        return 0, err
    }
    return x+y, nil
}

Because while err is reused, it does not shadow an outer err.
But this would be illegal:
func Example() (int, error) {
    x, err := thingOne()
    if err != nil {
        return 0, err
    }
    if x > 1 {
        y, err := thingTwo()
        if err != nil {
            return 0, err
        }
        x += y
    }
    return x, nil
}

Because the inner err shadows an outer one.
I think something like a shadow ban is necessary to prevent the footgun of accidental shadowing.

@carlmjohnson
What about this example?
func Example() (int, error) {
	x, err := thingOne()
	if err != nil {
		return 0, err
	}
	f := func() int {
		y, err := thingTwo()
		if err != nil {
			log.Println(err)
		}
		return y
	}
	return f(), nil
}

I think that having to call the error err2 is a small price to pay for making closures close over what you expect in other cases.

How about disable short declarations except the ones in the first part of control flow block?
Short declarations bring too many confusions than its convenience. They are much less readable than the long declarations.

Closing as dup of #377.

@ianlancetaylor #377 is "locked and limited to collaborators."

@jimmyfrasche Thanks.  I sincerely hope that #377 doesn't turn into a long-running discursive discussion.  I don't think that is helpful.


sesh connect does not consider space in dir names

What happened?
When I _, I expected _ but _ happened!
When I try to switch session with on of my zoxide path, (which have space in dir name). I expected to get into new session but sesh existed. I tried to replicate it manually below are the steps to reproduce the issue.
Step-1: create some dir with space in name
Step-2: z into that dir so zoxide will have entry for it
Step-3: try `sesh connect <dir_with_space_in_name> fails.
Example:

Execute below cmd to get all the session list and zoxide

sesh list -tz | fzf-tmux -p 55%,60% \                                      --no-sort --border-label ' Sessions ' --prompt '   ' \
--header '   all[^a] |  tmux[^t] |  zoxide[^x] |  find[^f]' \
--bind 'tab:down,btab:up' \
--bind 'ctrl-a:change-prompt(  )+reload(sesh list)' \
--bind 'ctrl-t:change-prompt(   )+reload(sesh list -t)' \
--bind 'ctrl-x:change-prompt(   )+reload(sesh list -z)' \
--bind 'ctrl-f:change-prompt(   )+reload(fd -H -d 2 -t d -E .Trash . ~)'

Try to select any dir which have space in name for me it was, see Notes Workspace have space

sesh connect ~/Library/Mobile Documents/iCloud~md~obsidian/Documents/Notes Workspace

Error:
➜  sesh connect ~/Library/Mobile Documents/iCloud~md~obsidian/Documents/Notes Workspace
2024/02/24 10:16:20 unable to connect to "/Users/ajay/Library/Mobile": failed to add "/Users/ajay/Library/Mobile" to zoxide: exit status 1
Hack/Workaround (manually)
I use single quote ' ' when i tried to connect with sesh and it worked.
➜  sesh connect '~/Library/Mobile Documents/iCloud~md~obsidian/Documents/Notes Workspace'
Version
sesh version 0.13.0
Relevant log output
Error:
➜  sesh connect ~/Library/Mobile Documents/iCloud~md~obsidian/Documents/Notes Workspace
2024/02/24 10:16:20 unable to connect to "/Users/ajay/Library/Mobile": failed to add "/Users/ajay/Library/Mobile" to zoxide: exit status 1
Reviewed

[X] Reviewed existing issues and made sure this bug has not already been submitted.
[X] I have read the documentation and understand how sesh works before submitting this bug

Sure, one more thing i observed when I use icons in my session and zoxide dir names,  name return by fzf is with icon so if we do something like delete session with ^d tmux does not find session bcoz name contains the icon too. Highlighted  here bcoz it might be similar issue like space.

Since fzf only accepts strings then the script would have to be updated significantly to support features like that.
Let me know if you have any ideas, it's getting pretty complicated trying to add icon support.

@ajaymahar I added space support for both session paths and names, download v0.14.0 of sesh and let me know if you have any issues.


Added setup files

Here, I added the setup files for the project, so we don't have to worry about it anymore!

LGTM! Nice work John! 🔥


Replace narrow non-breaking space with normal space

Replace non-breaking space with normal space because copying from site leads to confusion in some tools (e.g. InstallShield)

@microsoftshawarma
Can you review the proposed changes?
Important: When the changes are ready for publication, adding a #sign-off comment is the best way to signal that the PR is ready for the review team to merge.
#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@microsoftshawarma
Can you review this old PR and determine whether it needs to be closed or merged?

@microsoftshawarma
I'll merge this PR based on your approval. If this article requires further changes, please open a separate pull request.


Documentation for using GHCR in k8s mode for dapr init

What content needs to be created or modified?

Content related to dapr init -k using GHCR and image-registry flag should be added.
The related changes for self hosted mode has already been merged with this PR - https://github.com/dapr/docs/pull/2295
The associated pull request from dapr/dapr, dapr/components-contrib, or other Dapr code repos
https://github.com/dapr/cli/pull/984

/assign


Simplify deleting document collection groups

Rather than having the user delete the group items if there are any,
just handle that as part of deleting the group.
This also resolves an issue where a document associated with the group
with just one edition, which happens to be deleted, would prevent the
group from being deleted, as it's not visible to the user to remove
from the group.

Looks good - but I would also link to our current Trello, and also link to the commit that introduced the feature in the first place and didn't explain why they thought it was needed.

Sorry, I completely forgot to do this prior to merging :(


PeerConnection.Close doesn't always close the underlying turn client

Conversation carried on from: https://gophers.slack.com/archives/CAK2124AG/p1569602730038100

I unfortunately haven't been able to reproduce this. I am just getting familiar with the TURN code base though.
I am about ~1/3 done with adding TCP (and TCP/TLS) support now. As I work through the code base I am trying to cull as much as I can. If I see anything that jumps out at me will make sure to investigate more!

Ah! I forgot to come and close this as the PR in https://github.com/pion/ice/pull/111 fixes this - what was happening is this:

A peer connection is created between two peers
Local and remote descriptions are set properly
Communication ceases at this point

This left the controlled side of the connection in limbo as it was stuck waiting for candidates. Due to how the code was structured it never timed out so the connection stuck around forever. I had mistakenly believed I was actually calling the .Close method as I just assumed it would have received the timeout.
Since the controlled peer never actually timed out, I never received the Failed/Disconnected event and thus, in the way I was closing connections - never called .Close.
Once https://github.com/pion/ice/pull/111 is merged, this PR can be closed.


Problem with debugging wasm module compiled from C++

Hi,
I'm trying to use wasmtime's debug support. I followed the simple demo from https://github.com/bytecodealliance/wasmtime/issues/1613, and it works as expected. But when I try to debug a slightly more complex C++ program, there's some problem.
Here's the C++ snippet:
#include <string>

std::string hello(int x) {
    const char* s1 = "Hello";
    std::string s2 = "World";
    return s1 + s2 + std::to_string(x);
}

int main() {
    auto s = hello(12345);
    return s.length();
}

I compiled it with a freshly built clang (commit), coupled with prebuilt wasi-sysroot from wasi-sdk:
$ clang++ --target=wasm32-wasi --sysroot ./wasi-sysroot -fno-exceptions -g t.cc -o t.wasm

Then I use lldb (8.0.1) and a freshly built wasmtime (commit) to debug it:
$ lldb -- ./wasmtime/target/release/wasmtime -g t.wasm

Setting breakpoint and stepping thru statements all work fine. But somehow none of the local variables are printed as expected:
Process 26379 stopped
* thread #1, name = 'wasmtime', stop reason = step over
    frame #0: 0x00007ffff7fcf5af JIT(0x5555562ec490)`hello(x=0) at t.cc:6:12
   3   	std::string hello(int x) {
   4   	    const char* s1 = "Hello";
   5   	    std::string s2 = "World";
-> 6   	    return s1 + s2 + std::to_string(x);
   7   	}
   8
   9   	int main() {
(lldb) p x
(int) $0 = 0
(lldb) p s1
(WebAssemblyPtrWrapper<const char>) $1 = (__ptr = 0)
(lldb) p s2
(std::__2::string) $2 = {}

Any hints?
P.S. I'm doing all the experiments on Linux.

hen I use lldb (8.0.1) and a freshly built wasmtime (commit) to debug it.

At this moment recommended debuggers lldb-10 and gdb for various reasons/issues.

(WebAssemblyPtrWrapper) $1 = (__ptr = 0)

The wasm pointer is not really a pointer in computer memory that can be accessed. You may check https://github.com/bytecodealliance/wasmtime/pull/1482 on how to use helpers to unwrap wasm pointers in a debugger.

(WebAssemblyPtrWrapper) $1 = (__ptr = 0)

Not sure why s1 points to the memory offset 0. Did you try debugging non-optimized -O0 wasm?

Okay, I just check that with or without optimization -- the issue is the same.

Thanks for you reply :) @yurydelendik

At this moment recommended debuggers lldb-10 and gdb for various reasons/issues.

I retried the previous experiment with lldb 11.0.0 (freshly built) and gdb 8.2, respectively. Here are the results:
lldb
Process 101273 stopped
* thread #1, name = 'wasmtime', stop reason = step over
    frame #0: 0x00007ffff7fcf5af JIT(0x5555562ef450)`hello(x=0) at t.cc:6:12
   3   	std::string hello(int x) {
   4   	    const char* s1 = "Hello";
   5   	    std::string s2 = "World";
-> 6   	    return s1 + s2 + std::to_string(x);
   7   	}
   8
   9   	int main() {
(lldb) p x
(int) $0 = 0
(lldb) p s1
(WebAssemblyPtrWrapper<const char>) $1 = (__ptr = 0)
(lldb) p s2
(std::string) $2 = Summary Unavailable

gdb
Thread 1 "wasmtime" hit Breakpoint 1, hello (x=0) at t.cc:4
4	    const char* s1 = "Hello";
(gdb) n
5	    std::string s2 = "World";
(gdb) n
6	    return s1 + s2 + std::to_string(x);
(gdb) p x
$1 = 0
(gdb) p s1
$2 = {__ptr = 0}
(gdb) p s2
$3 = <incomplete type>

It seems switching debugger version doesn't help much.

The wasm pointer is not really a pointer in computer memory that can be accessed. You may check #1482 on how to use helpers to unwrap wasm pointers in a debugger.

Cool. I'll give it a try.

I looked at the issue. There are at least two problems:

[ ] TODO related to reference types (specific to C++) at https://github.com/bytecodealliance/wasmtime/blob/master/crates/debug/src/transform/unit.rs#L398
[ ] There is an issue with CL value tracking, which is a cause for wrong location in DWARF

@lei-april you can also turn off option at CL via --opt-level 0
I will try to address the above issues soon.

you can also turn off option at CL via --opt-level 0

Thanks. This does help :)
The result looks better now (with lldb 11.0.0):
Process 76848 stopped
* thread #1, name = 'wasmtime', stop reason = step over
    frame #0: 0x00007ffff7fc369e JIT(0x55555639a690)`hello(x=12345) at t.cc:6:12
   3   	std::string hello(int x) {
   4   	    const char* s1 = "Hello";
   5   	    std::string s2 = "World";
-> 6   	    return s1 + s2 + std::to_string(x);
   7   	}
   8
   9   	int main() {
(lldb) p x
(int) $0 = 12345
(lldb) p s1
(WebAssemblyPtrWrapper<const char>) $1 = (__ptr = 1024)
(lldb) p s2
(std::string) $2 = Summary Unavailable

(std::string) $2 = Summary Unavailable

C++ types with reference types in them will likely fail due to TODO mentioned above.


Update  to just call push

Re-made from https://github.com/rackt/react-router/pull/2658.
Tests will fail until https://github.com/rackt/history/pull/173 is merged and released.
Codemod: https://github.com/rackt/rackt-codemod/pull/5.
I'm still using context.history here - we could potentially merge this and release it before the API changes to get rid of the deprecation warnings for users, per https://github.com/rackt/history/issues/172#issuecomment-162336396.
I'm going to be extremely busy over the next week and a half or so, and someone else might have to shepherd this to release (docs, changelog, &c.), which is why this branch is on the main repo instead of my personal fork.

Re-made from https://github.com/rackt/react-router/pull/2658.
Tests will fail until https://github.com/rackt/history/pull/173 is merged and released.
Codemod: https://github.com/rackt/rackt-codemod/pull/5.
I'm still using context.history here - we could potentially merge this and release it before the API changes to get rid of the deprecation warnings for users, per https://github.com/rackt/history/issues/172#issuecomment-162336396.
I'm going to be extremely busy over the next week and a half or so, and someone else might have to shepherd this to release (docs, changelog, &c.), which is why this branch is on the main repo instead of my personal fork.

This probably needs an update to https://github.com/rackt/react-router/blob/master/modules/PropTypes.js#L10 to include push and replace as well.

I've got the same thing in a local branch, but it's for everything, not just Link. I also update to use this.context.router.
However, at the moment, the tests are littered with deprecation warnings from React checking contextTypes and how we've implemented the deprecation via a getter. I'm holding off for the useRoutes refactor until I land some of this, because it will be broken code until that's available.

@taion If you have any objections to my changes, let me know. Otherwise, it's :+1: from my end.

:shipit:
Do whatever you want w/r/t squashing or whatever. And let's merge https://github.com/rackt/rackt-codemod/pull/5 afterward.

Cool. Let me extract that function for creating the LD from to and then we should be good to go (and get the build green again! :smile:)

Squashed. Merging now!


Add GlobalMaxPool gradient tests

@rxwei the latest builds won't work. There's a test error as I mentioned in #467 .
/swift-apis/Tests/TensorFlowTests/TensorAutoDiffTests.swift:164: error: TensorAutoDiffTests.testTensorInitStacking : XCTAssertEqual failed: ("[1.0, 2.0, 3.0, 4.0, 5.0]") is not equal to ("0.0") -
/swift-apis/Tests/TensorFlowTests/TensorAutoDiffTests.swift:165: error: TensorAutoDiffTests.testTensorInitStacking : XCTAssertEqual failed: ("[ 6.0,  7.0,  8.0,  9.0, 10.0]") is not equal to ("0.0") -

Same build error in #471 as well.

@rxwei this test passed when we merged it last night. Did the toolchain get updated since then?

@eaplatanios so when you built it, they weren't getting tested. TensorAutoDiffTests were introduced in #469

Toolchain builds are blocked on TF-770.

@Shashi456 good catch. In that case we need to wait for TF-770 to be resolved as this test should only pass with a newer toolchain.
@rxwei oh that's the same issue I was having on my mac. Good to know it's reproducible.

Merged into master, this shouldn't fail anymore.

This can be merged as well.


Doc20210730

We will need to extend the coverage, but we have to capture the correspondences are they currently are.

This also fixes the staging branch. cc @kickoke


⚠ Mobitopup has degraded performance

In c5573d6, Mobitopup (https://napcard.voz.tech) experienced degraded performance:

HTTP code: 200
Response time: 11229 ms

Resolved: Mobitopup performance has improved in 9e32a65.


Unable to create the cache directory (/var/www/app/cache/prod)

Hi. Since the last update, I get this error on starting the toran-proxy container. I'm not sure whether this is caused by something in the container setup or the update of the toran-proxy version. I'm intending to dig into this a bit more later, but posting here first in case anyone's already made some progress.
My first guess is perhaps toran-proxy changed its cache location in a recent update and this new location needs creating in the Dockerfile.
Command: docker run cedvan/toran-proxy
Result: RuntimeException in log output.
Wed Jun  1 19:44:24 UTC 2016 - Starting synchronization...

  [RuntimeException]
  Unable to create the cache directory (/var/www/app/cache/prod)

Exception trace:
 () at /var/www/app/bootstrap.php.cache:2738
 Symfony\Component\HttpKernel\Kernel->buildContainer() at /var/www/app/bootstrap.php.cache:2699
 Symfony\Component\HttpKernel\Kernel->initializeContainer() at /var/www/app/bootstrap.php.cache:2478
 Symfony\Component\HttpKernel\Kernel->boot() at /var/www/app/AppKernel.php:51
 AppKernel->boot() at /var/www/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:70
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /var/www/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:123
 Symfony\Component\Console\Application->run() at /var/www/bin/cron:30

Wed Jun  1 19:44:24 UTC 2016 - Synchronization finished

hi @djbingham,
Thank you for feedback, I will analyze too;)

I checked with Jordi Boggiano, he thinks /var/www/app/cache/prod has always been written to by toran-proxy, as it's the standard cache directory for Symfony.

Ah, higher up the log output (didn't notice this earlier) I get:
Configure PHP...
Loading Nginx vhosts...
Configure Toran Proxy...
/scripts/toran-proxy/install/toran.sh: line 77: syntax error near unexpected token `fi'
/scripts/toran-proxy/install/toran.sh: line 77: `fi'
Starting Toran Proxy...

I see that you've fixed the issue I was about to point out: https://github.com/cedvan/docker-toran-proxy/commit/eea3493bd53e0e75db99dc00c9e95865b5204cfa

I'm now getting a different issue with the Toran Proxy sync process, although no longer any problems showing before Toran is launched:
Configure PHP...
Loading Nginx vhosts...
Configure Toran Proxy...
Installing Token Github...
Creating mirrors directories...
Installing Cron...
Loading permissions...
Starting Toran Proxy...
2016-06-03 12:27:23,646 CRIT Set uid to user 0

==========
Fri Jun  3 12:27:24 UTC 2016 - Starting synchronization...
Initializing private repositories
Writing packages.json for the private repository
Initializing packagist.org proxy repository

  [InvalidArgumentException]
  The "/var/www/web/repo/packagist/p" directory does not exist.

Exception trace:
 () at /var/www/vendor/symfony/symfony/src/Symfony/Component/Finder/Finder.php:656
 Symfony\Component\Finder\Finder->in() at /var/www/src/Toran/ProxyBundle/Service/Proxy.php:318
 Toran\ProxyBundle\Service\Proxy->cleanProviderFiles() at /var/www/src/Toran/ProxyBundle/Command/CronCommand.php:214
 Toran\ProxyBundle\Command\CronCommand->syncPublicPackages() at /var/www/src/Toran/ProxyBundle/Command/CronCommand.php:123
 Toran\ProxyBundle\Command\CronCommand->execute() at /var/www/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:259
 Symfony\Component\Console\Command\Command->run() at /var/www/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:860
 Symfony\Component\Console\Application->doRunCommand() at /var/www/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:192
 Symfony\Component\Console\Application->doRun() at /var/www/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:92
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /var/www/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:123
 Symfony\Component\Console\Application->run() at /var/www/bin/cron:30

toran:cron [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-s|--shell] [--process-isolation] [-e|--env ENV] [--no-debug] [--] <command> [<packages>]...

Fri Jun  3 12:27:25 UTC 2016 - Synchronization finished
==========

This seems to be fixed if I upgrade toran-proxy to 1.4.4, as per your open pull request: https://github.com/cedvan/docker-toran-proxy/pull/32.

When will this be merged in? Having same issue.

Having the same issue here also.

Fixed with https://github.com/cedvan/docker-toran-proxy/commit/f43ae1b12b10ec49c602a5fddd794ff1b5394f62


Entity-specific anonymization

I added functionality to specify the entities to be included in the anonymization for three anonymization types (context-preserving, named entity-based and combined). README and example (run.js) are adjusted accordingly. Moreover, I added a Logging class that might be helpful for further extensions. Please review and test carefully. Thanks!

@ben-aaron188 thanks for the review. I have added the remaining cases to the entities object and adjusted the version.

@ben-aaron188 et voilà. I adjusted the code accordingly. Please have a look and let me know what you think. Thanks!

Thanks @ben-aaron188! I did not cover the case that an entity contains a dot (in the example the input string contains the word "Amsterdam." instead of "Amsterdam" and hence the replacement did not work. Fixed this now. I also fixed another minor issue.


Update TypeScript exports to support moduleResolution bundler

moduleResolution: "bundler" is a recent setting added to TypeScript to support bundler setups. It's the setting you should probably be using when bundling code. With this setting TypeScript only searches for types in the exports field of the package.json.
It requires the following change:
 "exports": {
   "import": "./dist/index.js",
   "require": "./dist/index.cjs",
+  "types": "./dist/index.d.ts"
 },

This is issue is relevent to multiple @lezer packages I'm using.

Can you provide an example setup that demonstrates this problem? When I set up a simple project that uses "moduleResolution": "bundler" (+ strict mode) and has a typescript file using @lezer/common, it compiles without error.

I couldn't initially create a minimial repro with any Lezer package that caused TypeScript to throw an error, but I think I've figured it out now.
If the types and files are "siblings", e.g. index.js and index.d.ts, then TypeScript can resolve those without looking up the exports field. It seems all I needed to do was upgrade my Lezer packages which have now been updated to use "sibling" types. In older versions, for example @lezer/highlight, the module was at ./dist/index.js while the types were ./dist/highlight.d.ts such that<EMAIL_ADDRESS>required the exports field to be defined.
The error is usually something along the lines of:

There are types at '/path/to/package/dist/file.d.ts', but this result could not be resolved when respecting package.json "exports". The 'package' library may need to update its package.json or typings.

Apologies for the spam PRs across several Lezer packages. Populating exports field is not needed for those particular packages.


Gutter seems unaligned in >5.0.0

The gutter seems unaligned in our project. We pretty much only modified the color attributes.

V4.3.0 used to add these 2 attributes to .CodeMirror-gutter-wrapper:
position: absolute;
left: -28px;
V5.0.0+ no longer does. I found out that I could reposition it by adding position: relative to .CodeMirror-gutter-wrapper
Is anyone else experiencing this?

This is not happening in the demos. It is likely to be a CSS conflict or other CSS issue. Can you try and reduce your setup to the minimum HTML/CSS needed to cause this problem?

My bad, app wasn't fetching latest .css file from node_modules. Who know which css version we we're running with^^


Adding a comment is always associated with a review

Extension version: v0.92.0
VSCode Version: 1.91.0
OS: macOS Sonoma 14.5

Whenever I add a comment through the extension, it shows up as a review comment, see e.g. https://github.com/mne-tools/mne-python/pull/12707#pullrequestreview-2171610586. However, that's not what I want to do, and I think that's also not what is supposed to happen. What should happen instead is that a normal comment is added to the discussion.
Steps to Reproduce:

Check out some PR
Add a new comment
The comment is associated with a review, which should not happen in my opinion

I see what you're saying. We don't currently have a way to leave a PR review comment vs. a discussion comment.

Oh, thanks for clarifying, I didn't know that! It would be great to have of course!


[DNM] Checking downstream VA HCI trigger job flow

It is based on the work done by @son-vyas
https://github.com/openstack-k8s-operators/ci-playground/pull/12.

/trigger github-experimental

/trigger github-experimental

/trigger github-experimental

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b4760bb10ff04ab09764abdc69b29e61

/trigger github-experimental

/trigger github-experimental

After merging this config change  and retriggering the same via this comment.
Following buildset url got generated.
By checking the trigger job, We have review with origin pr link.
2024-08-22 07:53:55.992404 | TASK [Submit the Gerrit review]
2024-08-22 03:53:57.881700 | primary | remote:
2024-08-22 03:53:57.895406 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 03:53:57.895483 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 03:53:57.895505 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 03:53:57.895614 | primary | remote: Processing changes: refs: 1, updated: 1, done
2024-08-22 03:53:57.895638 | primary | remote: commit be09f28: warning: subject >50 characters; use shorter first paragraph
2024-08-22 03:53:57.895654 | primary | remote: commit be09f28: warning: too many message lines longer than 72 characters; manually wrap lines
2024-08-22 03:53:57.895668 | primary | remote: warning: be09f28: no files changed, message updated
2024-08-22 03:53:57.895682 | primary | remote:
2024-08-22 03:53:57.895696 | primary | remote: SUCCESS
2024-08-22 03:53:57.895710 | primary | remote:
2024-08-22 03:53:57.895724 | primary | remote:   https://review.rdoproject.org/r/c/tripleo-downstream-trigger-nested-virt/+/54154 Triggering VA HCI job by https://github.com/openstack-k8s-operators/ci-framew...
2024-08-22 03:53:57.895745 | primary | remote:
2024-08-22 03:53:57.895758 | primary | remote: The following approvals got outdated and were removed:
2024-08-22 03:53:57.895772 | primary | remote: * Verified+1 by zull-sf-hosted CI
2024-08-22 03:53:57.895789 | primary | remote:
2024-08-22 03:53:57.895803 | primary | remote:
2024-08-22 03:53:57.895817 | primary | To ssh://review.rdoproject.org:29418/tripleo-downstream-trigger-nested-virt
2024-08-22 03:53:57.895830 | primary |  * [new reference]   HEAD -> refs/for/master
2024-08-22 07:53:58.049269 | primary | changed

/trigger github-experimental

After merging Add depends-on in the trigger review commit message config change and triggering the experimental job again .
Here is the buildset url.
From the job log, Below review got triggered.
2024-08-22 08:10:25.384890 | TASK [Submit the Gerrit review]
2024-08-22 04:10:26.875884 | primary | remote:
2024-08-22 04:10:26.891406 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 04:10:26.891435 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 04:10:26.891442 | primary | remote: Processing changes: refs: 1, updated: 1
2024-08-22 04:10:26.891449 | primary | remote: Processing changes: refs: 1, updated: 1, done
2024-08-22 04:10:26.891455 | primary | remote: commit 0226f0f: warning: subject >50 characters; use shorter first paragraph
2024-08-22 04:10:26.891462 | primary | remote: commit 0226f0f: warning: too many message lines longer than 72 characters; manually wrap lines
2024-08-22 04:10:26.891469 | primary | remote: warning: 0226f0f: no files changed, message updated
2024-08-22 04:10:26.891476 | primary | remote:
2024-08-22 04:10:26.891482 | primary | remote: SUCCESS
2024-08-22 04:10:26.891488 | primary | remote:
2024-08-22 04:10:26.891500 | primary | remote:   https://review.rdoproject.org/r/c/tripleo-downstream-trigger-nested-virt/+/54154 Triggering VA HCI job by https://github.com/openstack-k8s-operators/ci-framew...
2024-08-22 04:10:26.891509 | primary | remote:
2024-08-22 04:10:26.891516 | primary | To ssh://review.rdoproject.org:29418/tripleo-downstream-trigger-nested-virt
2024-08-22 04:10:26.891523 | primary |  * [new reference]   HEAD -> refs/for/master
2024-08-22 08:10:26.945980 | primary | changed

Here is the commit message: https://review.rdoproject.org/r/c/tripleo-downstream-trigger-nested-virt/+/54154/4//COMMIT_MSG

/lgtm

/cherrypick 18.0.0-proposed


DTU Mesh for Evaluation table

Hi authors,
Thanks for your amazing work and quick replies to issues. I have a question about the Table 5 in the paper, about chamfer distance evaluation for DTU:

I'm wondering are the reconstructed meshes from MonoSDF in this table the same as those in the download script (https://s3.eu-central-1.amazonaws.com/avg-projects/monosdf/meshes.tar)? Because I used the meshes from dtu_allviews_mlp folder and ran eval.py under dtu_eval folder, but chamfer scores (last column in DTU.csv file, shown in the image below) cannot match with the 2nd last row in the table above.

Does it has something to do with the culling step? Below is the culled mesh for scan 65, it seems like some background mesh is not cleaned.

I'd really appreciate it if you can shed some light into this matter, thanks in advance!

@mwang625 Thanks for pointing this out. I think I put a checkpoint of scan37 that's trained for only 100 epochs. For all the other scenes, the quantitative results are similar which might be related to some training variants. You can download the meshes trained with 2K epochs here: https://drive.google.com/file/d/1Ty6Nu_sEHbGq6ZYALsM-oQ8Y1O_Pe22v/view?usp=share_link

Thank you so much for checking and uploading the mesh!


[Enhancement] Keep nether portal texture option

Not sure how it'd look but could be an interesting look with the other dimension one side of the pane blocks and your world on the other. This could also provide the option of lower spec pcs to use portal rendering 0 with this option to get seemless teleportation still, lmk ur thoughts!

Implemented https://qouteall.fun/immptl/wiki/Config-Options#enable-nether-portal-overlay

Implemented https://qouteall.fun/immptl/wiki/Config-Options#enable-nether-portal-overlay


Deliverable 1: PWA and Photo Upload Functionality

Progressive Web App (PWA) support
- Implement a fully functional PWA, ensuring compatibility across devices.
- Conduct thorough testing to identify any bugs or issues within the PWA.
- Once identified, these bugs should be fixed to ensure smooth operation of the app.
- The PWA should then be encapsulated within a Docker container for easy deployment and version control.
Photo Uploads
- Develop a feature to allow users to upload photos via an S3 bucket proxy to AWS servers.
- When a photo is uploaded, its address should be added to the respective note's JSON object.
- The uploaded media (photos) should be visible in the 'Edit Note' section.
- A preview of the uploaded photo should be displayed on the home page next to the corresponding note.
    Due Date - 06/15/2023

This was accomplished a while back. Closing issue as changes mentioned have been implemented. PWA support is available and photo uploads are also up and running.


Record implementation referencing Arrow buffer

If we were reading records from an Arrow buffer, either during ingest or compaction, with the intention of writing them to Parquet, we would like to be able to write to Parquet directly from the Arrow buffer. Right now we're converting them into an intermediate format using a HashMap in the Record class.
One problem with this is applying iterators, as we want them to be able to interact with records without worrying about the Arrow implementation, eg. to filter out records based on the value of a particular field. We could enable that to some degree by treating data on an Arrow buffer as a sequence of Sleeper Records.
In ArrowRecordBatchAcceptingRecords, we load Sleeper records into Arrow. We'd like to be able to read the values of those records from the buffer without reading the whole record into a HashMap.
Once we have an implementation for that, we could introduce an interface for reading values from a record, eg. with just the "get" method from the current Record class on the interface, or some similar method signature. Both the current Record class and the new class would implement that interface.
This is part of https://github.com/gchq/sleeper/issues/576
This can use the Arrow schema implementation from https://github.com/gchq/sleeper/issues/543. This will be needed for referencing Sleeper records read from Parquet into Arrow in https://github.com/gchq/sleeper/issues/547.

On hold pending review of approach to using Arrow.

Closing as https://github.com/gchq/sleeper/issues/576 will now be done after changing to use DataFusion for record processing.


RLP subcommand to decode BFT RLP extra data

A subcommand to Besu similar to the rlp encode but decoding to display the data in the rlp extra data. This would be very useful for displaying the details of an existing extra data to confirm it has the correct validators.
Acceptance Critieria

Add rlp decode command that will output the contents of the BFT encoded RLP extra data
Must support decoding of IBFT legacy, IBFT2 and QBFT extra data formats

I think this can be closed as PR https://github.com/hyperledger/besu/pull/6895 has merged


Unable to generate sample code due to util.format undefined

Customer reported sample code still not working. "Unable to generate sample code" is showing, and this is the error in the console.

yarn add util or npm install util in the repo where the problem appears will help.
It's a dependency required by httpsnippet library.

@audieleon here is a workaround for the Unable to generate request sample error you're seeing. ☝🏻
We'll leave this issue open until we find a way to solve this in all node environments, because it works in some and not in others.

It would be good to just add httpsnippet "node" dependencies to dependencies of elements-core: util and process. This would ensure that stuff works in most environments, I believe.
process is already added as dependency for Gatsby. After it's added to elements-core, it should be removed from there.

Additional context: the reason this error was so elusive to us is that webpack 4 adds those node dependencies by itself.
We use storybook and create-react-app a lot and both of them use webpack 4.
In fact, I believe we almost by accident use webpack 4 to build web components as well, which likely would be good to change. I'll create an issue for that.

yarn add util or npm install util in the repo where the problem appears will help.

Wanted to confirm for you that this worked on my project. (And I've incorporated the changes that removed webpack).
Thank you.


Scala Core: add schema matching for unversioned instances

See https://github.com/snowplow/iglu/issues/147
We want to find the highest matching schema version for an unversioned instance, where:

matching means that the instance passes validation against the given schema version
highest means that no schema versions with higher numbers pass

I think @fblundun has some ideas on a performant algorithm here - Fred could you share?

@chuwy - should this ticket move to Iglu Scala Client?

Migrated https://github.com/snowplow/iglu-scala-client/issues/53

I actually think it should belong to core. But It also depends on when do we need it. I don't have plans to migrate iglu client to core in near version. It can introduce breaking changes in SCE, so decided to be very careful on it.

Okay, will keep both open for now!

My original idea was to do a binary search for the last matching schema, instead of just checking starting at the most recent schema and working backwards. However I now don't think this is a good idea, because it assumes that for any two schemas A and B such that A is matching and B is not, A is higher than B. This is not necessarily true - in general I think it is only true given the additional assumption that A and B have the same model and revision.
I suppose we could use that information to reduce the number of schemas we check - e.g. if 1-0-9 isn't matching, we know that 1-0-8 and below also aren't matching. So for each model-revision pair, we need only check the latest addition.

Migrated to https://github.com/snowplow-incubator/iglu-scala-core/issues/28


Add Node::set_owned_by_parent() and bind new property owned_by_parent.

Close proposal #9282.

I decide to bind a new property owned_by_parent to decide a node is an internal node of parent or not and can be changed later by users, instead of exposing force_parent_owned() which can set it to true only.

I see no valid reason to be able to clear this value, and it creates a huge risk for unintended errors IMO

I see no valid reason to be able to clear this value, and it creates a huge risk for unintended errors IMO

Like my comment in #9282.
This change dose not change any existing logic, user who user this new feature should take responsibility for themselves (like EditorScript may lead to crash). So I don't think this change will bring any risk.
If I neglect something, please tell me, I will add a new field (may be named "owned_by_parent_override") to handle this feature.

This change dose not change any existing logic

It does, you can't clear this, only set it
Sure you can break things in ways, but I don't think we should add features that make it easier
What's the use case for clearing this value? The setting the value is reasonable, but there's no reason I can see to clear it

Yes, that's what I mean :) there's no way to set it to false currently, because it's not needed, or safe

Let's keep it as a method and not a property, there's no reason to clear it, and it is wrong in my opinion, there's no reason to ever clear it, it's not a matter of assuming, it's a matter of what this property is for and what it makes sense for
If you want to clear it all you need to do is reparent it


Measure post queue timings

This should measure how long a post has been in the bodyfetcher queue at the point that the API call for its site is made. It stores the timings locally in bodyfetcherQueueTimings.p so that stats can be analysed. Also created a Python script to do that analysis.
PR'd so I can get a code review, and cc @Undo1 since he was talking about doing this.

Just a note that this broke some things, too, so we had to revert it in commit 0177abf, but it's now fixed.  (dict and lists aren't interchangeable, and we have to do some manipulation to make things work proper)

@teward I nuked the bodyfetcher queue manually, because yeah it broke things.


API / debugAPI design

Right now, API is:

Mature features which are user-facing
They are not meant to expose the user to danger when exposed to the internet, but if we go live on mainnet, then upload/download API endpoints should be guarded

Right now, debugAPI is:

bee operator endpoint (get status of current bee node)
experimental and/or deprecated features
endpoints which we don't want to expose via API due to security reasons

Some features which are exposed from the debugAPI need to be exposed to developers to interact with the bee node from the browser (e.g. bee-status. @alsakhaev requested CORS on the debugAPI (#1280)
All together, the situation calls for an analysis for the different API routers we expose, their functionality and their protection.

sweet 🤙  can we generalise this to include also the api and gateway mode?

guys here are some sketch user roles i made a while ago.
there are a lot of ways to cut it but i think these are quite distinct and easy to reason with.

I am an infrastructure node operator using Bee to earn BZZ using my available resources.
I am a mobile/desktop app user using a local Bee proxy to provide ‘back end’ functionality of my app.
I am a web app user using a Bee gateway as the ‘back end’ functionality of my app.
I am a gateway operator providing Bee gateway services to web app users.
I am a developer of 1,2,3 or 4

I think the 3 points by @Eknir describe well the current situation and I can easily image that they provide a way to move forward.

The developer endpoints may only contain features that are used to help the development of Bee in some ways (e.g. testing). Normal users should not have it enabled and it should be difficult to enable CORS for these endpoints because they are potentially dangerous or destructive. No experimental or deprecated features belong here. This could be on a different port (e.g. 1635) and by default only exposed on localhost.

Experimental and deprecated features could be enabled on the normal API (e.g. 1633) but they could be enabled conditionally, for example by providing a command-line option.

All other endpoints could be exposed on the normal API but there could be "private" endpoints which require some form of authentication (e.g. an API key) to be accessed. One could also argue that actually all endpoints are such endpoints, because potentially all exposed operations (will) cost money at some point.

Two API requests:

expose network & chain ID https://github.com/ethersphere/bee-status/issues/6
health endpoints on both API and debug API ideally as websockets https://github.com/ethersphere/bee-status/issues/17

I spoke about this with Janos, and we concluded the following:

The API and debugAPI were designed in a specific way. I don't see a reason to get rid of this design at this point. debugAPI is intended for node operators, to inspect the status of their node. api is used by dapps, to interact with the Swarm network. @janos can explain the vision much better than I do here. We agreed that he will update the documentation. @janos , will you leave a comment here when this PR is out?

we need to change the name of API and debugAPI, as it is confusing and doesn't tell anything to the user. I suggest to change debugAPItonetworkanddebugAPItonode` (@janos, this is different than what we spoke about in our call, but I gave it some thought and would suggest the above. Let me know what you think).

the gateway-mode is not documented, neither in our docs, nor in our API documentation. I will make an issue (and link it here shortly), which requests this documentation.

Responding to @agazso :
1: there is currently only one such 'developer' endpoint (/chunk/delete). I agree about moving this away from the debugAPI, as this is not an operation which we want any user to do under normal circumstances. I asked @janos how/where this is currently used. With regards to testing, the vision of @janos is that integration tests should only mimick "normal" operators, and hence these 'developer' endpoints are not explicitly needed. Nevertheless, I can imagine the functionality of such endpoints for testing or research, and I would support creating a separate endpoint for this.
2: Good suggestion. There are currently no experimental and deprecated features. Will keep your suggestion in mind for the future.
3: We spoke about this with @janos and agreed that there is no fundamental difference between "normal" websites (which can incur bandwidth that costs/costed money) and web3 endpoints. Nevertheless, I sympathize with you here, as it might be better to be cautiousness than reckless in the beginning. @janos , wdyt about making all API endpoints protected by some authentication?

document gateway-mode issue: https://github.com/ethersphere/bee-docs/issues/163

3: We spoke about this with @janos and agreed that there is no fundamental difference between "normal" websites (which can incur bandwidth that costs/costed money) and web3 endpoints. Nevertheless, I sympathize with you here, as it might be better to be cautiousness than reckless in the beginning. @janos , wdyt about making all API endpoints protected by some authentication?

To me it seems that there is a difference between certain endpoints. Otherwise why are they disabled in gateway mode?

We are actively researching the permissioned API topic. I'm closing this in the meanwhile as some more ripe conclusions should emerge out of that very soon. A possible coalescing of both API is in the cards with role based APIs


Refactor function signatures

Result<T, PyObjectRef> -> PyResult
Make pymethod return types as small as possible

Wow, this is really nice, but also a lot of code. Did you do this manually or automated? Would it be possible to split this work into several pull requests?

Mostly manually. I will try to split it

If you can split it somehow, this would be good, otherwise no worries, I will look into it, but it will take me some time to view it all.

Super! Nice job!

Oh, it is merged without spliting, even more great :D


Import Not Working

I've been trying to add new books to Shelves 3.7.2 for over a month, single and batch additions. The app reports the book is found but it never resolves the "importing" or "adding" notice. I've experimented with Wifi and Data connections to determine if there is a difference, and there appears to be not.
There was a comment here in the issues about the failure to search due to a possible change in the Amazon API, which would explain the inability to download the title details, but either way, until this is resolved, the app is useless to me. I've been a paid user for many years and use it frequently to track various household inventory items. I love the ease of use and integration with Amazon and other data sources.
Thank you.

Same situation here with paid version. It seems that the web service used to find information on the web doesn't respond : the search after a scan wait keeps waiting.
If gjtorikian is not able/willing to keep it running, maybe the source code of this service could be released as open source to help this useful app continue.
Another option would be to add some web scrapping capabilities in Shelves itself.

@GJTorikian : if you don't plan to fix the online search service, you should warn the potential buyers of the Unlocker on Google Play! For those of us who already bought it, could you provide more information on the web or API sources that were used so that we may try to find alternative ways to continue to use this useful app?


T254212 Fix mouse leave event

Phabricator ticket : https://phabricator.wikimedia.org/T254212
Problem
The popup close immediately as soon as the mouse leaving the hover target element
Solution
FF doesn't have event.toElement, use event.relatedTarget instead
Note
it works on Firefox 77 / Firefox 58 (older)

Update and ready to be reviewed in FF.


simpleviewer: Show PDF in full width (without SCROLLBAR_PADDING)

In https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.js#L356, we find that the width scaling involves SCROLLBAR_PADDING (unless in presentation mode). For my use case, I would like the PDF to occupy the full width of the encapsulating div. (There's no scroll bar anyways.)
One obvious solution would be to add another switch value, 'full-width', which unconditionally sets the hPadding to 0.

@Snuffleupagus @yurydelendik Looking at the code where {h,v}Padding are set, it'd suffice for me if I could set
pdfViewer.presentationModeState = PDFJS.PresentationModeState.FULLSCREEN;

I'm not sure of the other implications of setting this to full screen when it really isn't, but anyhow it removes the unnecessary padding.
Unfortunately, I don't see PresentationModeState exposed anywhere. Can you help me out?

@nschloe I would very strongly advice against doing that, since that will have a number of (most likely) unwanted side-effects.
Please look through the code in pdf_viewer.js, try searching for e.g. .presentationModeState, and you will see that there's quite a bit of special handling of the FULLSCREEN case.
I don't think that we should expose two slightly different "width" zoom options in the standard viewer, which is why I personally don't think the proposed PR is a good idea in its current form. (Also, in the standard viewer, using full-width as implemented in that PR would cause a horizontal scrollbar to be displayed.)
Unfortunately the review times have been a bit long recently, so I don't think the developers have had time to look at it (and provide feedback) yet.
To add a full-width option, a very simple solution would be to modify pdf_viewer.js e.g. like this:
diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js
index ea70979..ec0d5cd 100644
--- a/web/pdf_viewer.js
+++ b/web/pdf_viewer.js
@@ -424,6 +424,11 @@ var PDFViewer = (function pdfViewer() {
               Math.min(pageHeightScale, pageWidthScale) : pageWidthScale;
             scale = Math.min(MAX_AUTO_SCALE, horizontalScale);
             break;
+          case 'full-width':
+            // Width zoom option without padding, for e.g. viewer components.
+            scale = this.container.clientWidth /
+                    currentPage.width * currentPage.scale;
+            break;
           default:
             console.error('pdfViewSetScale: \'' + value +
               '\' is an unknown zoom value.');

I've found an even simpler fix, see pull request #5787.

@Snuffleupagus Thanks for reviewing the pull request and highlighting why it isn't appropriate.
Anyhow, the problem of a too narrow width remains:

If I understand correctly, one problem is that pdf_viewer is hardcoded to appear with a border-shadow, requiring the extra padding. Would it be possible to remove this requirement? I could live with anything  that makes better use the precious screen real estate.

@Snuffleupagus Do you think the addition of 'full-width' to pdf_viewer.js as mentioned above would be an appropriate fix for this?

Since the full-width option wouldn't really deal with the underlying issue, which is the assumption that the pages have a border, I think that it would probably be better to try and make that configurable.
The following is a very quick, and poorly tested, patch that enables the user of PDFViewer to remove the page borders through an option: https://github.com/mozilla/pdf.js/compare/master...Snuffleupagus:noPageBorder.
Please test it and see if does approximately what you want, in which case I would be happy to submit a PR for the devs to consider.

@Snuffleupagus This change does exactly what I need.
Before:

After:

(Note: The shadow borders in the screenshot are introduced by myself a posteriori.)
This is with
var pdfViewer = new PDFJS.PDFViewer({
    container: element[0],
    viewer: element[0],
    noPageBorder: false
});

If promoted to a pull request, I'd recommend to name the variable setPageBorder have it default to true (instead of noPageBorder defaulting to false). This makes it a tad bit easier to read and understand.

I agree that the name setPageBorder would be a better name. I also think @Snuffleupagus' fix is pretty clean.

This issue concerns an edge case that does not apply to the standard viewer, which is why I believe that a name such as setPageBorder would be bad for a number of reasons:

The default value currently is, and should continue to be, that the pages have borders unless specified otherwise, hence such a name would be quite misleading. It suggests that you must set the variable for the borders to be used, when the reverse is actually correct.

Having a parameter that defaults to true would mean that if it's not provided when calling new PDFViewer(...), we have the somewhat illogical situation where an undefined value implies true (instead of the more expected false).

It would make the code slightly less clean, and given that this is an edge case, we should strive to make the implementation as simple as possible.

@Snuffleupagus Agreed. Not specifying the option should fall back to default (namely to set a page shadow), and since JS uses undefined as default and undefined is clearly false, we have to name the option omit.... Too bad, but I guess this is what JS is.


spr diff to update an existing PR that's not HEAD

Currently, if I use spr diff to update an existing PR, that PR must either correspond to the commit at HEAD, or I must pass --all, which treats all of the commits on my branch as a stack of dependent PRs and updates all of them.
It would be nice to be able to specify a specific commit in the branch and update just that commit's PR. To work around this, I can reorder the commits and move the one whose PR I'd like to update to HEAD, but it'd be nice not to have to do that.

Hi @adembo !
Your feature request is noted, but I won't be able to work on it so soon.
Anyway, there is another workaround. You can use git rebase --interactive and add a line x spr diff after the commit you want to update. This way you don't have to reorder anything.
See https://getcord.github.io/spr/user/stack.html for more details on that.