Pull Request Preview Environments for increasing maintainer productivity

I would like to make life easier for Payload maintainers by implementing Uffizzi preview environments.
Disclaimer: I work on Uffizzi.
Uffizzi is a Open Source full stack previews engine and our platform is available completely free for Payload (and all open source projects). This will provide maintainers with preview environments of their PRs in the cloud, allowing them iterate faster and reduce time to merge.
Uffizzi is purpose-built for the task of previewing PRs and it integrates with your workflow to deploy preview environments in the background without any manual steps for maintainers or contributors.
TODO:

[ ] Intial PoC

This sounds like a great use-case for a plugin.
I'm converting this to a discussion, as that is a better place for this.


Spelling fix in PAC definition

The word
PAC
Error report
Change spelling: separately instead of seperately.
Thanks for this site -- very helpful as I am taking on campaign finance duties!
Code of Conduct

[X] I agree to follow this project's Code of Conduct

Thanks. I think I just shipped that fix out. Should show up in a minute or two.

My little contribution ...
Thanks!


"adl_accounts.head" could list account but the structure is wield

Describe the bug
"adl_accounts.head" could list account but the structure is wield
To Reproduce
Steps to reproduce the behavior:

%reload_ext adlmagics
%adl login --tenant microsoft.onmicrosoft.com
%adl listaccounts --page_index 0 --page_account_number 30
adl_accounts.head

Expected behavior
Not sure, list account with clear UI or give out warning like "please use .head()"
And use the UI as samples

Screenshots

Desktop (please complete the following information):
OS: Win10
Browser: chrome
Jupyter Version: Python 3.6.2, Jupyter 4.4.0
Additional context
Add any other context about the problem here.

not sure is xx.head a valid function in pandas?


perf(runtime): Lazy evaluation of properties

Patch JSC to allow for dynamic property definition during assignment
(write access)

Make ObjCPrototype::materializeProperties a no op

Make ObjCPrototype::getOwnPropertySlot check metadata for properties,
before searching for a method

perf(metadata): Do not scan for property overloads
Properties cannot have "JS overloads" in base classes as methods do
and there's no need to traverse the hierarchy. JS overloads are methods
with different selectors and number of arguments which have the same jsName

PR Checklist

[x] The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
[ ] There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
[x] You have signed the CLA.
[x] All existing tests are passing: https://github.com/NativeScript/ios-runtime/blob/master/CONTRIBUTING.md#contribute-to-the-code-base
[ ] Tests for the changes are included.

test --performance

test --performance-only

test --performance-only

test --performance

test --performance

test --performance

test


fix markdown for C language

Markdown was not rendered correctly for C language.

You sure this is intentional?

No, please reopen PR.

You have to do that.


Simplify app.all and app.VERB

https://github.com/strongloop/express/blob/5.0/lib/application.js
Currently:
/**
 * Delegate `.VERB(...)` calls to `router.VERB(...)`.
 */

methods.forEach(function(method){
  app[method] = function(path){
    if ('get' == method && 1 == arguments.length) return this.set(path);

    var route = this.route(path);
    route[method].apply(route, slice.call(arguments, 1));
    return this;
  };
});

/**
 * Special-cased "all" method, applying the given route `path`,
 * middleware, and callback to _every_ HTTP method.
 *
 * @param {String} path
 * @param {Function} ...
 * @return {app} for chaining
 * @api public
 */

app.all = function(path){
  var route = this.route(path);
  var args = slice.call(arguments, 1);
  methods.forEach(function(method){
    route[method].apply(route, args);
  });

  return this;
};

But the following simplified code does exactly the same (in fact lib/Router/index.js does it):
/**
 * Delegate `.all(...)` and `.VERB(...)` calls to `Router#all(...)` and `Router#VERB(...)`.
 */

methods.concat('all').forEach(function(method){
  app[method] = function(path){
    if ('get' == method && 1 == arguments.length) return this.set(path);

    var route = this.route(path);
    route[method].apply(route, slice.call(arguments, 1));
    return this;
  };
});

It may seem different from the outside, but in fact is very different in the internal structure it creates. We can look into the consequences of changing this as part of 5.0.

Actually it changes the outside behavior as well; right now app.all only responds to known verbs. This change will make it also respond to unknown verbs.

Well, Router#all() already respond to unknown verbs, am I wrong?:
// create Router#VERB functions
methods.concat('all').forEach(function(method){
  Router.prototype[method] = function (path) {
    var route = this.route(path)
    route[method].apply(route, slice.call(arguments, 1))
    return this
  }
})

Right, you are not wrong. But app.all doesn't, so it would be a change of behavior for app.all

Since app.xxxx() is supposed to be a simple proxy to the main Router#xxxx(), shouldn't the behavior be the same?

In 4.x, it is not a simple proxy. Perhaps it can be in 5.0, but the behavior change just needs to be evaluated first.


Field is immutable

When trying to create a space as a user (not cluster-admin) I get the following error:
$ cat << 'EOF' | kubectl apply --kubeconfig joe_bloggs_config -f -
apiVersion: tenancy.kiosk.sh/v1alpha1
kind: Space
metadata:
  name: joes-space
EOF

The Space "joes-space" is invalid: metadata.annotations: Invalid value: map[string]string{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"tenancy.kiosk.sh/v1alpha1\",\"kind\":\"Space\",\"metadata\":{\"annotations\":{},\"name\":\"joes-space\"}}\n"}: field is immutable, try updating the namespace

kubectl auth can-i --kubeconfig joe_bloggs_config create Space
Warning: resource 'spaces' is not namespace scoped in group 'tenancy.kiosk.sh'
yes

@jicowan Thanks for opening this issue. Seems like a bug. To narrow it down: Can you try kubectl create instead of kubectl apply? Does that work?

create works.

@jicowan thanks for reporting, yes this was initially implemented to prevent users from overwriting annotations and labels defined in the space template in their account. kubectl create works because it is not setting any annotations, while kubectl apply does (for the three way merge later on). However, I changed this behavior so now that when annotations and labels are defined in the space template they will overwrite the ones provided by the user otherwise it is allowed to set annotations and labels.

@FabianKramm there is a similar effort underway in sig-multitenant with the tenant CRD and hierarchical namespace controller.  Are you working with the community on this effort?

@jicowan yes we are very aware of the tenant CRD and hierarchical namespace controller. While I think parts of the scope and goals of these projects are aligned with what kiosk is trying to accomplish, there are also quite some differences.
The basic primitive in the approach of the multi tenancy group is a tenant crd object and the corresponding tenant admin namespace that comprises all related resources to that tenant, such as allowed namespaces (tenant namespace crd), roles etc. The hierarchical namespace controller allows to define sync hierarchies between namespaces and basically allows some sort of templating for new and existing namespaces. When we first thought about kiosk and what it should accomplish we initially wanted to build upon these efforts from the multi-tenancy group, however we felt there are some concerns with the approach the multi tenancy took:

There were several problems from an users endpoint we wanted to improve on: for example, retrieving allowed namespaces an user has access to is not trivial with the approach the multi-tenancy group took, since the user at least needs to know its tenant admin namespace. In kiosk, you can easily run kubectl get spaces or kubectl get accounts as any user and see only the resources you have access to. This approach is more like how openshift is doing things with projects, which we believe is the superior approach instead of creating access resources (tenant namespaces) in each tenant admin namespace which takes additional space in the etcd.
There is an overhead in unused resources that are just there for templating and defining hierarchies but are actually functional in kubernetes and controllers will act on them: each tenant has one admin namespace and potentially additional resources (like resource quotas, network policies etc.) that will be synced to tenant namespaces. These resources are all functional itself and could slow down kubernetes controllers since caches get bigger and those resources also need to be reconciled. We feel this overhead is not needed and template resources should be separated from "real" resources.
Since the kubernetes sigs are a place to develop future standards for certain areas, we felt the pace these projects were moving for now was not fast enough for us. There are several efforts in the multi tenancy sig on the way, however we still feel that they are not mature enough to be built on. We are very aware that might change in future and we are very open to replace parts of kiosk with efforts from the multi tenancy group if they reach a more mature level and are widely accepted. We are also very open to help them in certain areas.

These concerns lead to the decision to build kiosk not on top of the efforts of the multi tenancy group for now. Nonetheless, these initial problems we had with their approach does not mean their efforts and our efforts are mutually exclusive and we might reconsider in the future. Besides the technical foundation, I think kiosk has a wider scope than what the multi tenancy group currently wants to build: e.g. multi cluster management and helm templates. So I think kiosk is definitely not designed as a competitor to what the multi tenancy group is doing, it is rather designed as a project that provides a single coherent (though opinionated) solution to the topic that might be build on some of the building blocks the multi tenancy group provides in the future.

@FabianKramm Thanks for the very thorough explanation. I like the CRD approach because they can be scoped to a namespace which I assume allows Kiosk to give tenants visibility to the resources, e.g. accounts and tenants, they "own".  It really wasn't clear from reading the KEP for the hierarchical namespace controller which resources would be synchronized with child namespaces.  I think it mentioned RBAC and network policies but then the tenant KEP seemed to imply that you could define other objects.  We should probably continue this discussion in another issue/thread since it no longer pertains to "field is immutable" issue I was having.  Thanks again for the response!


Support multiple image?

Does MiniCPM-V support multiple image? Asking because there are image comparison tasks in OpenCampus benchmark. How are those cases handled?

https://huggingface.co/openbmb/MiniCPM-Llama3-V-2_5/discussions/2#664c8889afe6e8c3a91aea25
Here's how it's handled.


Add transaction pool to Babe import queue

We need to submit report transactions when we discover an equivocation while importing (see here for definition of equivocation).
As an starting point, this PR adds the transaction pool to Babe import queue and check_header.

Please add a comment in the Babe code explaining why we're passing the transaction pool.
We merge too many PRs that adds code "for later", and we end up not using that code, and then nobody remembers why it's there nor dares to remove it.

I would go even further as @tomaka and say, why not build directly the full functionality? This is no big change, which can be reviewed together with the actual functionality.

I would go even further as @tomaka and say, why not build directly the full functionality? This is no big change, which can be reviewed together with the actual functionality.

The full functionality may be too much for a PR. But sure, I will push more code to give a bigger picture to the reviewers.

I added some more code, but looks like I can't go much further. The full functionality depends on #3063 and  #3137 at least.

This is not reviewable.  If you depend on other PRs the correct way to do it is to create a branch that descends from the one you depend on. FWIW I'm fine with merging just the changes to introduce the transaction pool to Babe (and Aura!) without depending on any pending PRs, provided that we comment it like @tomaka suggested.

I have not intention to merge the other PR, just to use it as a guide.
I'll follow the suggestions of @tomaka  and @andresilva  and just introduce the changes for the tx pool in Aura and Babe.

This is ready for review.

As discussed offline this is not usable as is, since the T type is not bound by anything. @marcio-diaz did some changes to fix this on another branch not sure if we wants to get them here or not.

T is useless with or without bound, and adding it doesn't look perfect either.
IMO, the point of splitting the other big PR was to facilitate review and merging.
If anyways we are going to merge this in a few days, then why not merging it now?

IMO my comment is justified, this PR is titled "Add transaction pool", and it adds a type T which does not relate at all to a TransactionPool, so as a logical unit of work it doesn't accomplish what it says it does. You added a trait SubmitExtrinsic here (https://github.com/paritytech/substrate/pull/3314) which solves this exact problem, my suggestion was to add something like that here in this PR. If that is done then in this PR we have a small set of changes and something that is logically correct (even though it is unused). I am OK with merging this anyway and just doing those changes in the other PR to avoid more work on this, but it has conflicts now.

@andresilva Thanks, conflicts solved.

@andresilva if you're happy with it, then please merge.


Specify where in config to put S2S pbjs.setS2SConfig

Hi,
I'm struggling to know if I've setup S2S right. Does it matter where in the config that I put pbjs.setS2SConfig? Thanks – might help others to specify this in the docs (if not the beginning or end than say anywhere.
Thanks

anywhere before requestBids is fine.

Thanks! I've put it just after pbjs.que.push inside the the prebid config section. BTW, is there any way I can test to verify that it's working? We just converted over to S2S from S2C. The ads are working but can't tell if the process is working as intended.

@pribeh thanks for filing this - I've added more explicit instructions to the docs, as well as more surrounding context in the code sample to that page:
Get started with Prebid Server
Let me know what you think!  If that makes sense to you, I'll close this out.

@rmloveland That's great! Thanks for being so helpful guys!
I found out something I missed. This might be off topic here, but mentioned in the docs is to "include Prebid Server" in the prebid.js file when downloading it from prebid.org/download. But I can't seem to find the option. I also don't see any code in prebid.js which would suggest the functionality is baked in by default. Am I missing something here?

@pribeh you're welcome!
On the other thing, good point -- I've just updated that section of the page to note that until we get the downloads page updated, you have to build from source to get the Prebid Server code.  (Hopefully the downloads page will get updated in a day or two.)

Thought so, thanks again @rmloveland! Please close the issue if you see fit!


fix firebase firestore import

Adds FirebaseFirestore to the example, otherwise an "unresolved identifier 'Firestore'" warning is thrown by Xcode

I signed it!

Thanks for the PR @crybat!
This change is harmless, but I'm unable to reproduce the error you're describing.

Well it might have been Xcode's cache. I tried just import Firebase again and no longer have the issue.


Support for namespaced models

Currently namespaced models generate table names with a / in them.  For example Foo::Bar becomes foo/bar which is not a valid dynamo table name as far as I can tell.

@adamhunter: Table names are constrained to /[a-zA-Z0-9_.-]+/ AWS Docs on Table name constraints. I think I'll add an error in migrations to prevent invalid table names, to match the regex. I'll convert Foo::Bar to foo.bar; my reasoning is naming conversions. TwoWords::Bar would become two_words.bar.


Fixed support email typo in readme file.

Fixed support email typo in readme file.

Thank you, but the email format is intentionally with at to help preventing spam (to some extend) by some scraping bots.


Add support for WarningsAsErrors for C# projects

Add the ability to specify certain warnings that should be treated as errors in C# projects

Merged, thanks!


RFC: Traits and wire format 2.0

I'm opening this issue to collect discussions that are scattered across many issues in a single coherent proposal.
Goals
The goal is to improve code size (#456), while fixing #455 #457 #458 as a bonus. The other main metrics (wire data size, and speed) shouldn't be sacrificed.
Better code size requires simpler code. Less things to do means smaller code.
In particular, making formatting require no state leads to the biggest code size wins from my experiments in #456. Currently defmt stores some state in InternalFormatter. This state has to be stack-allocated, initialized and passed around at every log callsite, which requires a nontrivial amount of instructions.
Proposed changes
1. Use rzcobs to encode the stream.
The wire stream contains many zero bytes, rzCOBS can compress them very fast. This offsets wire size increase from removing LEB128 encoding (proposed below).

Advantage: Improved wire size. In a test it reduces wire size by 38%
Advantage: More resilience to corrupted stream. Since rzCOBS has builtin framing, if a frame corrupts the decoder can recover and keep decoding the next frames.
Disadvantage: Slower. In the same test it's 10% slower. Maybe it should be made optional so the user can choose.

2. Remove LEB128 encoding

Advantage: Faster.
Advantage: usize/isize will now be fixed width, which allows for interesting optimizations later.
Disadvantage: Almost no code size gain, since LEB128 encoding is not inlined.
Disadvantage: Bigger wire size, but it's offset by rzCOBS.

3. Remove bool compression
Unfortunately it's simply not possible to do without state. Bools will be encoded as a single
byte. false = 0x00, true = 0x01. At least 0x00s will be compressed by rzCOBS.

Advantage: Faster.
Advantage: Smaller code (because no state).
Disadvantage: Bigger wire size, but not that much (printing many bools is not that common, somewhat offset by rzCOBS).

4. Apply "needs_tag" optimization to "first level" only.
Naively, array/slice contents would be encoded as tag+data+tag+data+... This wastes
space for the redundant tags, since all array items are the same type, which should theoretically
have the same tag.
defmt currently has a "needs_tag" optimization that skips writing tags for all items other than
the first, so the encoding is tag+data+data+data.... This applies recursively to all nested
tags.
However it has proven tricky to guarantee inner tags are really the same. Enums broke it in the past (#123),
but it also breaks with dyn (#458) and with manual Format impls (#455).
Solution:

Explicitly require that a given type MUST always have the same tag (Format trait change, see below).
Apply the optimization only to the "first level": [T] is encoded as T_tag + data + data + data, but
datas have their inner tags kept.

Manual Format impls are specially handled (see below).

Advantage: Fixes #455 #457 #458.
Advantage: Smaller code (can be implemented with no state).
Disadvantage: Bigger wire size, but only for types that have nested structs/enums. Basic slices such as [u8] have the same wire size.

5. New Logger trait.
pub unsafe trait Logger {
    /// Acquire the global logger.
    ///
    /// Never fails. Panics if already acquired.
    fn acquire();

    /// Releases the global logger.
    ///
    /// # Safety
    /// Must be called exactly once for each acquire()
    unsafe fn release();

    /// Writes `bytes` to the destination.
    ///
    /// This will be called by the defmt logging macros to transmit encoded data. The write
    /// operation must not fail.
    ///
    /// Note that a call to `write` does *not* correspond to a defmt logging macro invocation. A
    /// single `defmt::info!` call can result in an arbitrary number of `write` calls.
    ///
    /// # Safety
    /// May only be called after acquire(), before the corresponding release()
    unsafe fn write(bytes: &[u8]);
}

There are 2 main changes:

No dyn Writer anymore. The dyn Writer required being stored somewhere during the log call, which requires state, which causes bad code size.  This has been PR'd in #258.
acquire() can't fail, it now panics if called reentrantly. This is to avoid a conditional at every log callsite. Previous callsites had to do this:

if acquire() {
    log_stuff();
    release();
}

With this, callsites are now:
acquire();
log_stuff();
release();

which is somewhat smaller and easier to optimize.
Note that this still allows Logger impls that allow lockless logging at multiple "contexts" (priority levels, threads). They can simply allow acquire() to succeed multiple times, at most once for each "context". Some discussion in #258.
Rationale for completely disallowing reentrant acquires:

It's simpler.
It's rare: I haven't found any code in the wild that does reentrant acquires.
If you do a reentrant acquire by accident, you would like to know. With the previous design, the reentrant log frame is silently dropped, which is not fun to debug.

..

Advantage: Smaller code (because no state).
Disadvantage: the Logger trait is now more lowlevel, so harder to implement.

6. New Format trait.
trait Format {
    // Get the tag value. Must always return the same tag for the same type.
    fn format_tag() -> u16,
    // Write
    // safety: the global logger must be acquired.
    unsafe fn format_data(&self);
}

Format is not supposed to be implemented by end users manually. Only by macros or in-crate impls. To allow changing Format again in the future without breaking changes, the functions could be #[doc(hidden)] with an "implementation detail, don't use" rustdoc.
Format is now not object-safe. This solves #458. This is somewhat unfortunate. It could be made object safe with fn format_tag(&self), but then the "Must always return the same tag for the same type." doesn't hold. I tried tricks like impl Format for dyn Format but thatt's not allowed since dyn Format auto-implements Format. If there's a way around it I'd love to hear it. Using dyn Format is very rare anyway.
Why not const TAG: u16? Because the pointer casting to assign IDs to tags doesn't work in const context.
The separate format_tag() allows impls for [T] to print the tag once and then all the datas (the "needs_tag-lite" optimization).
Now the tricky question: how to handle custom format impls?
1: attribute:
#[derive(defmt::Format)]
#[defmt(format="{a=u8}.{b=u8}.{c=u8}.{d=u8}")]
pub struct Ipv4Address {
    u8 a,
    u8 b,
    u8 c,
    u8 d,
}

Advantage: As wire-efficient as an automatically derived impl.
Disadvantage: Much less powerful: limited to printing fields, can't do arbitrary calculations to print summaries/lengths etc.
Disadvantage: Requires a not-yet-invented syntax to specify fields to print, unclear how would that work. Would it allow printing fields of fields? indexing arrays?

2: Macro'd impl:
pub struct Ipv4Address {
    u8 a,
    u8 b,
    u8 c,
    u8 d,
}

defmt::impl_format!(Ipv4Address, {
    // user can do arbitrary logic here

    // impl_format! macro enforces there's exactly one write! statement.
    defmt::write!("{=u8}.{=u8}.{=u8}.{=u8}", self.a, self.b, self.c, self.d);
});

Advantage: As wire-efficient as an automatically derived impl.
Advantage: Powerful: user can print result of any calculation.
Disadvantage: Very ugly IMO. The "enforce only one write!" could be surprising, although with good error messages it might not be that bad.

3: CustomFormat trait:
pub struct Ipv4Address {
    u8 a,
    u8 b,
    u8 c,
    u8 d,
}

impl defmt::CustomFormat for Ipv4Address{
    fn format(fmt: Formatter<'_>) {
        // it's possible to do multipe writes!
        defmt::write!(fmt, "Ipv4Address({=u8}.{=u8}.{=u8}.{=u8}", self.a, self.b, self.c, self.d);
        // and conditional writes!
        if self.is_broadcast() {
            defmt::write!(fmt, " (broadcast)");
        }
        defmt::write!(fmt, ")");
    }
}

This is powered by a special impl impl<T: CustomFormat+?Sized> Format for T. Wire format is a "special tag" indicating this is a CustomFormat, then multiple writes, then a terminator tag.
Trait names up for bikeshedding. Maybe it should be RawFormat+Format, instead of Format+CustomFormat.

Advantage: Most similar to the defmt 0.2 way. Upgrading code is trivial.
Advantage: Extremely powerful: user can print result of any calculation, do multiple writes, do conditional writes. Fully matches expectations of users coming from core::fmt.
Disavantage: Not as wire-efficient as an automatically derived impl.
Disavantage: Having 2 traits might be confusing..?

4: A combination of the above?: Maybe do CustomFormat impl now, and add a more optimized way like the attribute or the macro later?

thanks for putting this together! this looks great.

Sections 1-4

these sound good

New Logger trait.
Never fails. Panics if already acquired.

this would be part of the trait contract and not a defmt guarantee, right?
that is the implementer of Logger::acquire should trigger a panic on a re-entrant acquire. they could as well choose to execute an abort instruction, which may result in less code size.

New Logger trait.
acquire() can't fail, it now panics if called reentrantly.

this sounds like it could turn out quite nasty depending on the chosen panicking behavior.
if the user uses a panic handler crate that uses defmt but doesn't handle re-entrancy gracefully they could end with infinite recursion, e.g.
#[panic_handler]
fn panic(info: &PanicInfo) {
    // this SHOULD use an `AtomicBool` to not call `error!` in presence of nested panics but doesn't

    // implicitly calls `acquire`
    defmt::error("{}", Debug2Format(info));
}

struct Bomb;

impl Format for Bomb {
    fn format(&self, _: Formatter) {
        let x = u32::MAX + 1; // unintentional panic with `dev` profile
        // ..
    }
}

defmt::info!("{}", Format);
// `info!` calls `acquire`
// then `info!` calls `Bomb::format`, which panics
// panic handler's `error!` calls `acquire` which is a "nested one" so that panics
// panic handler is invoked again; the new panic handler calls `error!` again
// another `acquire` = a new panic, repeat

I think this wouldn't be an issue with the current -> Option<dyn _> signature.
but to be fair the root of the problem is the panic handler not handling re-entrancy at all -- you can also shoot yourself in the foot with panic!("{:?}", Bomb) even if you don't use defmt at all.

New Format trait.
It could be made object safe with fn format_tag(&self), but then the "Must always return the same tag for the same type." doesn't hold.

I'm not sure I follow the conclusion. the &self-less version of format_tag also cannot guarantee 'must always return the same tag for the same type' because one can write:
impl Format for MyType {
    fn format_tag() -> u16 {
        static X: AtomicU16 = AtomicU16::new(0);
        X.fetch_add(1, Ordering::Relaxed)
    }
}

or do you mean that code generated by defmt proc macros can not uphold 'must always return the same tag for the same type' when trait objects are involved if format_tag takes &self but it can uphold that when format_tag is &self-less?

New Format trait.
The separate format_tag() allows impls for [T] to print the tag once and then all the datas (the "needs_tag-lite" optimization).

since omitting the tag for each element of a slice' falls under "optimization", and not "correctness", could we use the Hash::hash_slice "pseudo-specialization" trick for that optimization? Namely, have a single trait like this:
trait Format {
    fn format(&self, _: Formatter);
    // default implementation applies a tag per element
    fn format_slice(slice: &[Self], f: Formatter) {
        for element in slice {
            Self::format(element, f)
        }
    }
}

// user code
#[derive(Format)]
struct MyStruct { .. }

// expands into
impl Format for struct MyStruct {
    fn format(&self, f: Formatter) {
        // same generated code as today
    }

    // override the default implementation
    fn format_slice(&self, f: Formatter) {
        // use some unstable API to omit the tag of each element
    }
}

the downside is that manual implementations of Format (i.e. that do not use #[derive(Format)]) won't get the slice optimization
I think it'd be best if we can avoid the format_tag method in the API. If we can't then having 2 traits with one completely #[doc(hidden)] seems fine to me.

I think it'd be best if we can avoid the format_tag method in the API.

to clarify: my goal is not to make the trait object safe
given that we don't correctly handle dyn Format today (see #458) I would be OK with saying "we don't currently support trait objects" (by e.g. intentionally making the trait not-object-safe) for now if it proves tricky to get that working correctly

Never fails. Panics if already acquired.

this would be part of the trait contract and not a defmt guarantee, right?
that is the implementer of Logger::acquire should trigger a panic on a re-entrant acquire. they could as well choose to execute an abort instruction, which may result in less code size.

Yep, by "never fails" I meant it doesn't return Option or Result. It can "fail" by panicking, and impls should panic on reentrant acquires.

if the user uses a panic handler crate that uses defmt but doesn't handle re-entrancy gracefully they could end with infinite recursion, e.g.

True... In my project I have added a "force release" function that the panic handler calls to ensure the logger is released, then logs the panic message. If the wire has framing it results in a corrupted frame, but the panic message's frame should always be transmitted correctly. I don't think this should be part of the trait, it rules out impls that don't do framing. Or maybe it should, but be documented as "best effort" as in it may cause corruption but you may still want it to avoid recursive panic death.

or do you mean that code generated by defmt proc macros can not uphold 'must always return the same tag for the same type' when trait objects are involved if format_tag takes &self but it can uphold that when format_tag is &self-less?

Yeah. Even if format_tag's contract says 'must always return the same tag for the same type' and all impls uphold it, it doesn't hold with dyn Format, which impls Format. So the code generated by the macros can't rely on the contract for any T: Format
The self-less version is not object-safe, so dyn Format is not a thing and everything is fine.
So if dyn support is a non-goal, let's drop it.

could we use the Hash::hash_slice "pseudo-specialization" trick for that optimization? Namely, have a single trait

Interesting! I totally didn't know about that. However: if the end-user gets to manually-implement the main format method, how do we prevent #455 #457?

Hmm. Following the lines of "pseudo specialization with default trait methods", maybe this works!

Advantage: single trait
Advantage: slice optimization applies to all impls, derived or custom.
Disadvantage: the trait is ugly... has doc(hidden) methods that the user must not impl or call.

trait Format {
    // Only used for custom format impls
    fn format(fmt: Formatter<'_>);

    // Get the tag value. Must always return the same tag for the same type.
    #[doc(hidden)]
    fn _format_tag() -> u16 {
        return internp!("{_custom_format}")
    }

    // Write
    // safety: the global logger must be acquired.
    #[doc(hidden)]
    unsafe fn _format_data(&self) {
        self.format();
        defmt::export::write_custom_format_terminator();
    }
}

impl<T: Format> Format for [T] {
    fn format(&self, f: Formatter) {
        // never called
    }

    fn _format_tag() -> u16 {
        return internp!("{[:?]}")
    }

    unsafe fn _format_data(&self) {
        defmt::export::write_tag(T::_format_tag());
        defmt::export::write_usize(self.len());
        for t in self [
            i._format_data();
        ]
    }
}

// =============== derived format example
// user code
#[derive(Format)]
struct MyStruct { .. }

// expands into
impl Format for struct MyStruct {
    fn format(&self, f: Formatter) {
        // never called
    }

    fn _format_tag() -> u16 {
        return 1234;
    }

    // Write
    // safety: the global logger must be acquired.
    #[doc(hidden)]
    unsafe fn _format_data(&self) {
        defmt::export::write_whatever();
    }
}

// =============== custom format example
// user code

impl defmt::Format for Ipv4Address{
    fn format(fmt: Formatter<'_>) {
        // it's possible to do multipe writes!
        defmt::write!(fmt, "Ipv4Address({=u8}.{=u8}.{=u8}.{=u8}", self.a, self.b, self.c, self.d);
        // and conditional writes!
        if self.is_broadcast() {
            defmt::write!(fmt, " (broadcast)");
        }
        defmt::write!(fmt, ")");
    }
}

Yep, by "never fails" I meant it doesn't return Option or Result. It can "fail" by panicking, and impls should panic on reentrant acquires.

Got it. Let's make sure the contract ("should $X") is documented in the API docs / book.

In my project I have added a "force release" function that the panic handler calls to ensure the logger is released, then logs the panic message.

sound handy, can we have that in the panic-probe panic handler? :-)

Yeah. Even if format_tag's contract says 'must always return the same tag for the same type' and all impls uphold it, it doesn't hold with dyn Format, which impls Format.

ooh, I think I see what you mean. if you encode [dyn Format] and try to do the 'omit tag' optimization it would be wrong because each trait object could, behind the opaque trait object, be a different type and such each one should be tagged. some spit balling: perhaps always avoiding the 'omit tag' optimization on [dyn Format] would make supporting dyn Format possible -- don't know if the 'dyn Format MUST be tagged' requirement shows up in other places though.

So if dyn support is a non-goal, let's drop it.

yeah, I'd be OK with saying "we don't trait objects at the moment (but may in the future)". dyn Format trait objects don't seem like something that would have a lot of use right now (if dyn A + B was supported in the general case -- not limited to auto-traits -- perhaps that would be different) so I'd be OK with punting on that until there's more demand for it.

Hmm. Following the lines of "pseudo specialization with default trait methods", maybe this works!

I'm not sure this produces a different _format_tag per type when manually implemented
trait Format {
    // ..
    // Get the tag value. Must always return the same tag for the same type.
    #[doc(hidden)]
    fn _format_tag() -> u16 {
        return internp!("{_custom_format}")
    }
}

e.g. X and Y (below) would have the exact same tag that comes from the default Format::format_tag method, no?
struct X { /* fields */}
impl Format for X {
    fn format(&self, f: Formatter) {
        // log fields, etc.
    }
}

struct Y { /* fields */}
impl Format for Y {
    fn format(&self, f: Formatter) {
        // log fields, etc.
    }
}

would that be a problem?
(also I'm not quite sure I see the need for _format_tag given that you can always override _format_data in #[derive]-d implementations and the implementations for primitives that reside in the defmt crate)

perhaps always avoiding the 'omit tag' optimization on [dyn Format] would make supporting dyn Format possible

It seems that's impossible without specialization. For example, impl Format for [&dyn Format] conflicts with impl<T: Format> Format for [T]. I haven't found a way to make that work.

I'm not sure this produces a different _format_tag per type when manually implemented

The requirement is only same type -> same tag. It's OK to have different types have the same tag: that doesn't break the needs_tag optimization.

the need for _format_tag given that you can always override _format_data in #[derive]-d implementations and the implementations for primitives that reside in the defmt crate

It's to implement the needs_tag optimization without state. _format_tag returns the tag, _format_data writes the data without the tag. This allows the caller to choose when the tag is printed. For single formats, caller will write tag+data. For multiple formats, caller will write tag+data+data+data+...
To improve code size, the goal is to remove InternalFormatter. This means we can no longer store a needs_tag bool there.

Thinking more about it, the "needs_tag" can be a bool param. That'd get passed in in a register, so it wouldn't cause bloat at every log call for stack-allocating anything.
The trait would look like this. It feels somewhat cleaner. I'm not sure which is better for code size and for speed, I can try both.
trait Format {
    // Only used for custom format impls
    fn format(fmt: Formatter<'_>);

    // Write tag if with_tag, then write data.
    // safety: the global logger must be acquired.
    #[doc(hidden)]
    fn _raw_format(&self, with_tag: bool) { .. }
}

I like the single trait with doc(hidden) methods approach over the macro and 2 trait options. Given that the implementation-detail methods are hidden I don't have any preference over having 1 or 2 so I would vote for the option that performs better.
I would suggest naming the doc(hidden) method(s) e.g. _do_not_override_this_raw_format. Rust-Analyzer can auto-complete default methods, but by default it will only auto-complete the non-default methods. In case someone does one code-assist too many then the method name will them to not override the method.

status update: by my count, 5 of the 6 proposed changes have been implemented :tada:

[ ] Use rzcobs to encode the stream.
[x] Remove LEB128 encoding #507
[x] Remove bool compression #507
[x] Apply "needs_tag" optimization to "first level" only. #508
[x] New Logger trait. #505
[x] New Format trait. #508

rzCOBS encoding is now also implemented, so closing this!


[doc] [R-package] Add {lightgbm} to ModelDeployment CRAN view?

Summary
CRAN supports a feature called "task views", which can be used to discover packages related to a specific topic.
@terrytangyuan, could {lightgbm} be added to the ModelDeployment view at https://cran.r-project.org/web/views/ModelDeployment.html, since {xgboost} is there? I know I could have DM'd you, but thought it would be better to have the conversation here so other LightGBM maintainers and users of the R package could see.
Thanks for considering it!

Thanks for asking and yes it would be great to include lightgbm there!
Feel free to submit a PR to the repo for this specific task view here. Note that once the PR is merged, it may still take quite a while for the change to be updated on CRAN since I will need to check with CRAN team to see if there's any update on the maintenance process over the last year.

Oh thanks! Didn't realize there was a repo dedicated to that task view, I should have scrolled all the way to the bottom of the page 😂

I've open https://github.com/terrytangyuan/ctv-model-deployment/pull/6 with a suggestion, so I think this issue here can be closed. Thanks again!

Sweet! LightGBM is on https://cran.r-project.org/web/views/MachineLearning.html as well.


Binding expression characters mess up WPF data grid

Some binding expression characters cause an error when WPF tries to render the data preview grid.  From testing, these cause errors:  [..] and (

Possible solutions

Strip any problem characters from header
Prohibit these characters from the header
Escape problem characters

Related stack overflows - here and here

We should investigate how others are solving this in VS.

Setting it to priority:0 because it's crashing VS now

Add warning on data page that file header cannot be used.


Remove each comment's leading whitespace in comment::rewrite_comment

Fixes #5391
Previously, leading whitespace at the start of custom comments (CommentStyle::Custom) would lead consume_same_line_comments to not return the correct byte position for the end of the comment.
This issue cascaded into a situation where an extra indented newline was inserted into rustfmts output causing a "left behind trailing whitespace" error.
Removing leading whitespace helps to properly identify the end of the comment and properly rewrite the comment.

Just want to elaborate a little more on the issue that we're experiencing here, and why I think this is the approach we should take. For reference here's the code for identify_comment:
https://github.com/rust-lang/rustfmt/blob/3de1a095e0ed52ade1b88d165d44d80455d3e6c5/src/comment.rs#L269-L408
Taking the example from the original issue identify_comment gets called with orig: &str = "        //.Y\n". when we compute the style (line 276) we determine that this is a CommentStyle::DoubleDash comment.
When hitting the match on line 321 we enter the first match arm because we just computed the style as CommentStyle::DoubleDash. This means we call consume_same_line_comments with style = CommentStyle::DoubleDash, orig = "        //.Y\n", and line_start = "// ".
Now lets zoom in on the for loop in consume_same_line_comments:
https://github.com/rust-lang/rustfmt/blob/3de1a095e0ed52ade1b88d165d44d80455d3e6c5/src/comment.rs#L305-L317

the if statement returns false since the trimmed line isn't empty
the else if statement returns false since "//.Y" doesn't start with "// ", and when we recompute the comment style we determine the trimmed comment to be CommentStyle::Custom.
Lastly we hit the else case and immediately break out of the loop and return (false, 0) from consume_same_line_comments.

Now on line 364 we split the original string at 0, which gives us ("", "    //.Y\n"),
When we finally get to the last conditional statement at the end of identify_comment we end up hitting the else case which recursively calls identify_comment on the trimmed version of rest, which is "//.Y\n".
By trimming the comment up front we save ourselves from doing additional work that would lead us to recursively call identify_comment with a trimmed version of the string anyway.

Confirmed this PR works for me on Gentoo with rust 1.78.0 AND with locally built rustfmt[^1] from latest commit c97996fab61fc09432abcf64dbe3ce2d84f4d679
diff --git a/src/main.rs b/src/main.rs
index 316bc87..65dfb1a 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,7 +1,7 @@
 mod my_error_things {
     //const CUSTOM_ERROR_MSG_BUFFER_SIZE: usize = 6;
     pub const CUSTOM_ERROR_MSG_BUFFER_SIZE: usize = 4096; // one kernel page?!
-                                                          //^ must be pub due to being used in a macro!
+    //^ must be pub due to being used in a macro!
 }
 fn main() {
     println!("Hello, world!");

Also it passes cargo test locally, so I guess it didn't break anything that was under tests.
Thanks!
[^1]: though it was kinda difficult to run the locally built rustfmt since it's linked dynamically which was surprising to me, so I had to run it like this:
$ LD_LIBRARY_PATH="/home/user/.rustup/toolchains/nightly-2023-12-28-x86_64-unknown-linux-gnu/lib:/home/user/.rustup/toolchains/nightly-2023-12-28-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" /tmp/rustfmt/target/x86_64-unknown-linux-gnu/debug/rustfmt --config version=Two src/main.rs


Add goto implementation

Should jump to implementation if there is only one, otherwise show a list (maybe use the error window / location list)

ALT+F12 does this. Should this be closed?

@ctolkien, that's go to definition - this one is still valid / something that needs to be done

Reffing #106


Reload series count stat at startup

Required for all non-trivial PRs

[ ] Rebased/mergable
[ ] Tests pass

Fixes an issue where the _internal.shard.seriesCreate stat gets lost on server restart.

LGTM 👍


Fix remaining warnings

This PR:

Addresses remaining Error Prone warnings.
Adds Durations2, DurationStringifier, TimestampStringifier, and EnumStringifier. These classes were copied and adjusted from Time, and will be removed there when migrating to newer dependency of base.

Other notable changes:

Abstract base for FieldValidator tests was introduced.

@armiol, PTAL.


Fold not matching VSCode

So remove the underline and replace the bg with a matching color right?

I will make a PR late tonight to adjust this.


Delete related whitespace when removing unused param

Include adjacent whitespace in the text edits to remove the
parameter declaration and its occurences in calling code.
https://github.com/rust-analyzer/rust-analyzer/issues/6663

lgtm!
bors r+
On Monday, 21 December 2020, Rüdiger Herrmann<EMAIL_ADDRESS>wrote:

Include adjacent whitespace in the text edits to remove the
parameter declaration and its occurences in calling code.
#6663 https://github.com/rust-analyzer/rust-analyzer/issues/6663
You can view, comment on, or merge this pull request online at:
https://github.com/rust-analyzer/rust-analyzer/pull/6987
Commit Summary

Delete related whitespace when removing unused param

File Changes

M crates/assists/src/handlers/remove_unused_param.rs
https://github.com/rust-analyzer/rust-analyzer/pull/6987/files#diff-ba85305bb7edb6035fadfb703d33c5dd42001310b0495ebe9878cc3faf655b53
(81)

Patch Links:

https://github.com/rust-analyzer/rust-analyzer/pull/6987.patch
https://github.com/rust-analyzer/rust-analyzer/pull/6987.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-analyzer/rust-analyzer/pull/6987, or unsubscribe
https://github.com/notifications/unsubscribe-auth/AANB3MYHLYXUDG2RXVYMOV3SV6C4PANCNFSM4VEQ6FQA
.