net/url: empty query value is dropped on Encode()

Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (go version)?
1.9
https://play.golang.org/p/FWtvu_9XEM
The Values.Encode function adds an unnecessary = to the end of kv pairs, even if the original value was empty.  When using flag style url query values, this is kind of annoying because it is supposed to be either present or absent.
It would be nice to interpret a url.Values with nil value slice as a flag, and skip the = for encoding.

To clarify, there are two related issues going on:  ParseQuery and Encode don't round-trip the string.  I don't think this is technically possible (we don't know if = should be kept), but it is possible to pick a better lossy translation.
Trying to work around this behavior by explicitly setting the value to nil:
var v url.Values
v["prev"] = nil

doesn't produce any output.

We probably can't change the v["prev"] = nil behavior to start generating output. That would probably break enough people. A lot of code & tests have accumulated over 5 years depending on every quirk of our implementations.

It would be nice to interpret a url.Values with nil value slice as a flag, and skip the = for encoding.

Using nil or even a sentinel []string value would be unable to represent ?foo&foo&foo: https://play.golang.org/p/nO_OlSCiCE
You'd almost need a sentinel string value, but strings in Go are compared by value, which means we'd need to make up some magic value like:
package url

// EmptyValue is blah blah no equals sign.
const EmptyValue = "\x00\x00"

But then that precludes any user from actually using that value.
We did something like this for https://golang.org/pkg/net/http/#TrailerPrefix but TrailerPrefix worked because a valid value can't contain a colon, so we have a safe value to use.
With URL parameter values, all values are unacceptable.

Perhaps an empty sentinel array?  It would be hacky no doubt.  Something like:
var sentinel [0]string

func EmptyValue() []string {
  return sentiel[:]
}

Then, in Values.Encode, it could check if the address of slice[0]  matches the address of sentinel.   Thoughts?

Again, that doesn't help with https://play.golang.org/p/nO_OlSCiCE

Depending on what level of Hacks are passible, I could imagine creating special strings with a prefix, and then slicing them to put in url.Values,  When encoding, reflect and unsafe could be used to look for the sentinel prefix to know if its a special empty:
https://play.golang.org/p/hbj9XZEDEl

Sorry, we will not stoop to that level.

I don't think that's stooping.  If there is a valid technical reason for disallowing it that would be an acceptable answer.  The core of the problem is url.URL is publicly defined to be a map.  Without defining a new type, or special parameters, the only remaining solution is to modify behavior.  That is what the original CL does.
That said, supported foo&foo&foo is not a primary goal of the CL.  Only that specific by more common problem needs a solution.

I don't think that's stooping.

You may not, but I do, and so would most of the Go team. We avoid unsafe except in very low-level places. The net/url package is not low-level and we don't want to make it be low-level by using unsafe.

That said, supported foo&foo&foo is not a primary goal of the CL. Only that specific by more common problem needs a solution.

It seems weird to support x=y&foo but not foo&foo. I'd prefer to be consistent, even if that meant the current foo=&foo= behavior.

Ok, expanding the scope of solutions.  Please rank these in the most acceptable order:

Declare that this issue is not a real problem, ignore it (The "ostrich" algorithm)
Fork net/url, put it in x/net/url, which causes package proliferation
Overload Encode to accept a func(p string) bool, that decides if it should be empty or not.
Overload Encode to accept a func(ps []string) []bool, to allow deciding particular empties (like foo&foo=&foo
Add a BetterValues, which is a [][2]*string that can accurately describe a query.
Modify Encode to interpret nil or empty slices as an indicator that it should not be present
Modify URL to have a ForceEmptyParam which changes how it encodes.  There is precedent for adding such booleans to URL.
Other?

3, 4, and 5 are API creep, 1 is not a real solution, because one of the other will take its place.

Commentary:

This hasn't been a problem for 5 years. I'm a little surprised it suddenly is. Which applications require ?foo without an = sign?
We've stopped expanding x/*. See #17244.
We can't overload Encode. That's an API change.
We can't overload Encode. That's an API change.
Gross.
Ideal (not changing API), if we figure out which way causes the least breakage in the community.
Maybe. But force which? All? And that only helps (*URL).String, not https://golang.org/pkg/net/url/#Values.Encode, which is probably okay.
Put it on github.com/carl-mastrangelo/carlurl

Eliding the = makes for shorter urls.  This is a good thing. I'm actually kind of surprised this isn't possible today or even tested for, given how many other good API decisions were made in net/.

You can see why I picked option 6 CL, because it was the best.

is not feasible.  By making my own, I force it to be on my API surface, just like Values is on URL, which is itself in http and elsewhere.

Looking at the Go 1 Compatibility promise, this appears to be undefined behavior that I am proposing to change.  If some very small number of tests break, can we proceed with the original change?

We'll discuss at a @golang/proposal-review meeting.
Maybe @dsnet could test the change inside Google (a pretty large codebase) and run all those tests and see how much breaks.

Assigning to myself to go test this.

Can you elaborate why this is an important problem to solve? Like Brad said, it's been five years and has not been a problem. Your original report says only "kind of annoying". We tend not to make major possibly-breaking changes with lots of downside if the only upside is to eliminate "kind of annoying".

Two answers to your question:  First, it's a good thing, and second it's okay to make this change.
Why is it a good thing?

Foremost is symmetry; parsing and encoding roundtrip doesn't preserve the original string. https://play.golang.org/p/SDU98LLL9v I realize that due to url.Value being publicly defined as a map, there is no way to express this in general (the a&a&a case), but this brings symmetry much closer, and for a likely use case.
Shorter URLs are good.  They are easier to read, easier to share, take up less space on the wire.
There isn't a good way to work around it.  url.Values doesn't provide me with a way of expressing what I want.  It gets 95% of the way there.  If you wanted to remove the superfluous '=' characters, how would you do it?  A regex?  String indexing a byte slicing?  At that point you might as well have not used url.Values at all.  The overhead of a minor tweak forces you to throw the baby out with the bathwater.

The reason I even raised this was because of issue 3; I couldn't easily work around this.   Reimplementing URL encoding is a bad use of my time, error prone, a code smell, breaks compatibility with the standard library (like with http.PostForm), and even goes against core values of Go:
https://golang.org/doc/faq#x_in_std says:

provide key functionality that many Go programs require, such as formatted I/O and networking. It also contains elements important for web programming, including cryptography and support for standards like HTTP, JSON, and XML.

This is absolutely something important for web programming; it affects the visual representation of how my site links together.
Why is this an okay thing?
Why is this a safe change to make?

I would like to point out there prior to my PR, there were no tests for this case.  Perhaps the idea never came up, or the original authors hadn't anticipated the use case.  My change would have passed without any tests being added at all.
The behavior is not specified in documentation at all.  As in nowhere.  Not in Go, not in the URL RFC.   This does not violate the Go compatibility promise.
The reason no one has complained?  Perhaps lack of usage.  I only could a handful of uses in Go itself.  Every usage I did see used this as k=v.   I consider this an unanticipated use case.

We tend not to make major possibly-breaking changes with lots of downside if the only upside is to eliminate "kind of annoying".

So far that's FUD.  We don't know who will break, if anybody.   That is why I specifically asked:

If some very small number of tests break, can we proceed with the original change?

Which I believe @dsnet said he would try inside of Google after 1.9 is out the door.   I would do this myself, but I am unfamiliar with the Go import process internally.  Changing the files and testing globally affected very few tests, implying that the source files aren't actually used.  That's neither here nor there.

I asked why the problem is important to solve. The only part of your response that could be viewed as addressing that question is the "shorter URLs are good" line. That sounds like a "nice to have" level importance. Let's work with that.
Round-tripping is not a justification. In general there are multiple possible URLs mapping to any particular representation. For example x=1&y=2 and y=2&x=1 map to the same representation, but no one complains that Encode cannot return the latter. Arguing that a particular form should round-trip is really just arguing that "this is the preferred printing form out of all the equivalent ones."
In this case, "x=" and "x" both map to the same representation, url.Values{"x": {""}}, and Encode prefers to print "x=". If the problem is that shorter URLs should be preferred, it sounds like you are arguing that Encode should instead choose "x"; that is, when value is empty in key=value, the = should be omitted as well. But I don't even see that on your list of choices. Why not?
The solution (6) introduces an unexpected special case that doesn't follow from the general one. It's established that url.Values{"x": {"1", "2"}} means x=1&x=2 and url.Values{"x": {"1"}} means x=1. In general, the URL has len(q["x"]) x parameters. The obvious extension is that url.Values{"x": {}} means no x parameters, not one without an equal sign. Even if no existing tests break, it's a special-case that will likely trip people up for years to come. That cost only makes sense to solve a very important problem. I don't see that level of importance here.

In this case, "x=" and "x" both map to the same representation, url.Values{"x": {""}}, and Encode prefers to print "x=". If the problem is that shorter URLs should be preferred, it sounds like you are arguing that Encode should instead choose "x"; that is, when value is empty in key=value, the = should be omitted as well. But I don't even see that on your list of choices. Why not?

It isn't as clear from this issue, because the CL was sent for review before this issue: https://go-review.googlesource.com/c/46834/1/src%252Fnet%252Furl%252Furl_test.go
As mentioned in the comment, I wanted to make the minimal possible change to enable the behavior.

The solution (6) introduces an unexpected special case that doesn't follow from the general one.
It's only unexpected if constructing the url.Value from the map functions rather than the methods.  Using Get, Set, Parse, Add, etc.  can be made to keep existing behavior.

In the CL I sent out, I use len(vs) to check if the value is empty, but it doesn't have to.  It could check vs == nil to mean that the = could be removed.  This preserves the most original behavior, because ParseQuery always puts in a zero length slice ( https://play.golang.org/p/QyvlwYsVlK ).  url.Values produced by the library would appear unchanged, while allowing my own code to set v[key] = nil, to override the production of = chars.
I do agree it is a special case, but since there isn't any wiggle room left in the API, it seems the most reasonable.  That said, I disagree it is unexpected.  It would be impossible(?) to encounter using solely the methods in net/url.  The only surprising thing would be if someone today had code that looked like:
v, _ := url.ParseValues("q=1")
v["q"] = nil
v.Encode()

and expected the result to be empty.

Even if no existing tests break, it's a special-case that will likely trip people up for years to come.

If it doesn't trip up the entire corpus of Go code written in the past 5 years, then I find this statement to fall flat.
Finally:

Round-tripping is not a justification. In general there are multiple possible URLs mapping to any particular representation.
This isn't a black and white issue.  It isn't like all round tripping is possible, or none of it is.  What I am asking for in this issue is to add a class of url param strings that previously were not round trippable, which subsequently can be.    If I wanted 100% fidelity of query strings, I would have sent Option 5 out for review; I didn't.

It doesn't appear the Go team feels this is an issue.  I'm closing this.


<IP_ADDRESS>

https://ci.appveyor.com/project/jpboyce/cb-cis-windows-2016#L1615

Resolved by 9cc4dbe


Add support to replace parameter with multiple different types (Like Generic parameter?)

As in issue #8 sugestet, it may be nice if parameter and other Types could be replaced with different types.
From original issue
	[Mixin(typeof(Shared), nameof(Test), nameof(Test) )] // `class MixinAttribute( Type mixin, params String[] placeholderValues )`
	public partial class Test
	{
	}

	public class Shared
	{
		public static implicit operator Int32( [Placeholder(0)] Shared self )
		{
			return 1;
		}

		struct PlaceholderFoo {
			Shared GetShared() { throw new NotImplementedException(); } // or use an interface?
		}

                [TextPlaceholder(1, typeof(PlaceholderFoo))]
		[return: Placeholder(0)]
		public static Shared operator+( [Placeholder(0)]  Shared left, [Placeholder(0)] Shared right )
		{
			PlaceholderFoo foo = new PlaceholderFoo();
                        return foo.GetShared();
		}
	}

When the mixin is copied into the receiver class, all parameters (and return-types) tagged with PlaceholderAttribute(1) are
lexically replaced with "Test" (as it's specified in [Mixin(typeof(Shared), nameof(Test) )]) while type-names (or any text in general)
inside a member function can be specified using the hypothetical attribute [TextPlaceholder] while the method uses real
types (in this case struct PlaceholderFoo) which gets replaced with the second value from the receivers params
String[] placeholderValues, so the above is compiled to this:

    public partial class Test
    {
        public static implicit operator Int32( Test self )
        {
            return 1;
        }

	public static  Test operator+( Test left, Test right )
	{
		Test foo = new Test();
		return foo.GetShared();
	}
    }

This approach has the benefit of allowing the "template" source to still compile as valid C# (so we can avoid using T4) while >allowing more powerful functionality closer to C++ templates - without introducing the limitations of C#'s generics as a means >of parameterisation (as mentioned below).

ToDo

[ ] More specifications

I'm not sure yet for what input is needed and what output expected. If I understand it correctly PlaceholderFoo is some kind of template. Maybe an Interface or another (or the same) mixin.

Yeah, I was iterating over ideas as I was writing my post and I didn't provide more details about how struct PlaceholderFoo was meant to work.

So here's the idea for struct PlaceholderFoo:

The motivation is that we need parameterized mixins:

Given that C# doesn't support multiple-inheritance for class types - but does support multiple interface implementation and reimplementation - compile-time mixins that interact with their new containing type can only work if the name of the new containing type can be used inside the mixin, which necessitates the need for mixin template type-name placeholders.

(A type-name placeholder being a specific type of template parameter - as mixin templates could have other parameters for things like names, literals, other identifiers, etc)

Non-parameterized mixin templates otherwise behave the same as copying+pasting the body of one class into another, which restricts many useful applications of mixins because of C#'s various rules; a good example is implementing operator overloading.

So given that we need parameterised mixins, why not just use a C# generic parameter as a placeholder?

Because C# generics are very limited: a C# generic class cannot derive-from nor implement a generic type argument, nor can a type parameter be used to represent this (this worsens C#'s problem of not supporting covariant return types... for now). You also cannot specify default types for generic parameters either.
But the main reason to not use generics is that it then restricts you from making a mixin template that actually is generic (because then it's unclear which generic parameters should be considered as template parameters as opposed to generic type parameters that should be preserved in the rendered mixin).

Instead, we could represent placeholders in the mixin template body with actual (non-generic) types - but using dummy types (typically this is an empty struct with no members).

Using dummy-types to exert more control over C# generic monomorphism is a pretty powerful technique; for example, you can use this approach to supply additional non-type-name parameters to a C# generic type, but that's another topic).
Using actual types means we can specify things like required or expected interfaces, members, even their inheritance tree - just like with real C# generics and still get our mixin template code to compile.

So the canonical example is implementing operator overloading - I'll demonstrate the approach using a dummy placeholder type (the struct PlaceholderFoo type) with the canonical tedious example of implementing IComparable<T> and overloading comparison operators:
partial class ComparableMixin : IComparable<ComparableMixin>, IEquatable<ComparableMixin>
{
	public static Boolean operator==(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return true;
		if( left is null || right is null ) return false;
		return left.Equals( other: right ); // Invoke IComparable<T>.Equals, not Object.Equals, btw
	}

	public static Boolean operator!=(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return false;
		if( left is null || right is null ) return true;
		return !left.Equals( other: right );
	}

	public static Boolean operator<(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return false;
		if( left is null ) return true;
		if( right is null ) return false;
		return left.CompareTo(right) < 0;
	}

	public static Boolean operator<=(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return true;
		if( left is null ) return true;
		return left.CompareTo(right) <= 0;
	}

	public static Boolean operator>(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return false;
		if( left is null ) return false;
		if( right is null ) return true;
		return left.CompareTo(right) > 0;
	}

	public static Boolean operator>=(ComparableMixin left, ComparableMixin right)
	{
		if( left is null && right is null ) return true;
		if( left is null ) return false;
		if( right is null ) return true;
		return left.CompareTo(right) >= 0;
	}

	public Boolean Equals( ComparableMixin? other ) // IEquatable<ComparableMixin>.Equals
	{
		if( other is null ) return false;
		Boolean isEqual = default;
		this.EqualsImpl( other, isEqual: ref isEqual );
		return isEqual;
	}

	public Int32 CompareTo( ComparableMixin? other ) // IComparable<ComparableMixin>.CompareTo
	{
		if( other is null ) return 1;
		Int32 cmp = default;
		this.CompareImpl( other, cmp: ref cmp );
		return cmp;
	}

	partial void EqualsImpl( ComparableMixin other, ref Boolean isEqual );

	partial void CompareImpl( ComparableMixin other, ref Int32 cmp );

	public override Boolean Equals(Object? obj) // C# complains if we implement IEquatable without overriding Object.Equals
	{
		if( obj is ComparableMixin other )
		{
			return this.Equals( other: other );
		}
		else
		{
			return false;
		}
	}

	public override Int32 GetHashCode() // C# complains if we override Object.Equals without overriding Object.GetHashCode
	{
		Int32 hash = default;
		this.GetHashCodeImpl( hash: ref hash );
		return hash;
	}

	partial void GetHashCodeImpl( ref Int32 hash );
}

Note that I assume that the mixin system would not only copy the class's body to the destination type, but also copies over the supertype and declared implements-interfaces parts.

Some problems with the above example:

First-and-foremost: all occurrences of "ComparableMixin" inside the mixin template always needs to be replaced with the destination consuming type (as an invariant type) - however users might want some occurrences to be replaced with
To avoid compile-time errors for not implementing IEquatable<T> or IComparable<T> the mixin must fully implement those interfaces, but must still hand-off control to the destination type for the real implementation - which means needing to use partial void EqualsImpl to stub it out - which then means the destination mixin consumer needs to also implement methods with the exact same signature but without the benefit of an interface or real partial method to ensure the signature matches.

(TODO: Finish writing this post...)

I'm not sure what this paragraph means:

But the main reason to not use generics is that it then restricts you from making a mixin template that actually is generic (because then it's unclear which generic parameters should be considered as template parameters as opposed to generic type parameters that should be preserved in the rendered mixin).

In the End every parameter must be replace sooner or later and it is the desision of the user of the type to decide when.
This is not different from other generics.  E.g.
public class MyDirectory<T> : Dictionary<T,string> { }

To avoid compile-time errors for not implementing IEquatable or IComparable the mixin must fully implement those interfaces, but must still hand-off control to the destination type for the real implementation - which means needing to use partial void EqualsImpl to stub it out - which then means the destination mixin consumer needs to also implement methods with the exact same signature but without the benefit of an interface or real partial method to ensure the signature matches.

I'm not sure which specific version of c# you need for that feature and if it also need a specific dot net verions, but if you add an modifier to the partial method, the compiler ensures that the method is implemented. You can also use return types in that case and don't need a ref parameter
  public partial int GetHashCodeImpl();

The only problem with that usage is that the Mixin class itself needs an implementation. For that reason I added a feature that will replace abstract methods with partial ones.
So with the current implementation of Mixins your sample would look like that:
    [Mixin.Mixin(typeof(ComparableMixin))]
    public partial class Test
    {
        public partial bool EqualsImpl(Test other) => false;

        public partial int CompareImpl(Test other) => -1;

        public partial int GetHashCodeImpl() => 1;
    }

    abstract class ComparableMixin : System.IComparable<ComparableMixin>, System.IEquatable<ComparableMixin>
    {
        public static Boolean operator ==([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null || right is null) return false;
            return left.Equals(other: right); // Invoke IComparable<T>.Equals, not Object.Equals, btw
        }

        public static Boolean operator !=([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null || right is null) return true;
            return !left.Equals(other: right);
        }

        public static Boolean operator <([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null) return true;
            if (right is null) return false;
            return left.CompareTo(right) < 0;
        }

        public static Boolean operator <=([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null) return true;
            return left.CompareTo(right) <= 0;
        }

        public static Boolean operator >([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null) return false;
            if (right is null) return true;
            return left.CompareTo(right) > 0;
        }

        public static Boolean operator >=([Mixin.Substitute] ComparableMixin left, [Mixin.Substitute] ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null) return false;
            if (right is null) return true;
            return left.CompareTo(right) >= 0;
        }

        public Boolean Equals([Mixin.Substitute] ComparableMixin? other) // IEquatable<ComparableMixin>.Equals
        {
            if (other is null) return false;
            return this.EqualsImpl(other);
        }

        public Int32 CompareTo([Mixin.Substitute] ComparableMixin? other) // IComparable<ComparableMixin>.CompareTo
        {
            if (other is null) return 1;
            return this.CompareImpl(other);
        }

        public abstract bool EqualsImpl([Mixin.Substitute] ComparableMixin other);

        public abstract int CompareImpl([Mixin.Substitute] ComparableMixin other);

        public override Boolean Equals(Object? obj) // C# complains if we implement IEquatable without overriding Object.Equals
        {
            if (obj is ComparableMixin other)
            {
                return this.Equals(other: other);
            }
            else
            {
                return false;
            }
        }

        public override Int32 GetHashCode() // C# complains if we override Object.Equals without overriding Object.GetHashCode
        {
            var hash = this.GetHashCodeImpl();
            return hash;
        }

        public abstract int GetHashCodeImpl();
    }

There are two problems with the current implementation and this sample. First the interfaces will be copied exactly as written withoud fully quallifieing it. So in the Sample above I added the System namespace to mitigate this.
The seccond is that the SubstitueAttribute does not support Generic Parameters. So the type there will also not be replaced.
Which raises the question, does it need the Attribute at all? Or do we always want to replace the own Identifier with the concrete type?

I don't think an empty struct as an placehoder will work. You can't do anything with it in the mixin. Since an empty struct does not have any members, you can't call any method on it. I think the only valid Approach for replacement types are other mixins.
(You could use refleation, but I don't like that aproach, and you would still be able to use it, when you use Generic arguments.
For that the mixins themselves maybe should be marked with an attribute. In that case you would not need to define the different replace parameters on the mixin, but every type that is used and is decorated with the mixin attribute needs to be replaced.
On the consuming side you can then make an mapping of mixin to actual type that should be used.

I noticed some problems when I tested some implementations.

You can't have attributes when referencing classes with generic parameters where you fill them out

//                        Valid                                                  Invalid
//                          |                                                       |
//                          V                                                       V
    class ComparableMixin<[Mixin.Substitute] T> : IComparable<ComparableMixin<[Mixin.Substitue]T>>

An Attribute Argument cannot use type parameters

//                                   INVALID
//                                      |
//                                      V
    [Mixin.Mixin(typeof(ComparableMixin<T>))]
    public partial class Test<T>

The first problem can be removed if we no longer use the SubstituedAtribute but replace every Mixin automaticly. For the second problem we could use some replacement structs. Something like struct TypeParameter1. That is replaced with the first type parameter. We can have some predefined structs, but the user needs to define its own, since it must satisfy the constrains of the generic type parameter.

@Jehoel I fixed some bugs so your sample should work with the current implementation. (Version 1.0.52)

Sample Code

Mixin:
    abstract class ComparableMixin : IComparable<ComparableMixin>, IEquatable<ComparableMixin>
    {
        public static bool operator ==(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null || right is null) return false;
            return left.Equals(other: right); // Invoke IComparable<T>.Equals, not Object.Equals, btw
        }

        public static bool operator !=(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null || right is null) return true;
            return !left.Equals(other: right);
        }

        public static bool operator <(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null) return true;
            if (right is null) return false;
            return left.CompareTo(right) < 0;
        }

        public static bool operator <=(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null) return true;
            return left.CompareTo(right) <= 0;
        }

        public static bool operator >(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return false;
            if (left is null) return false;
            if (right is null) return true;
            return left.CompareTo(right) > 0;
        }

        public static bool operator >=(ComparableMixin left, ComparableMixin right)
        {
            if (left is null && right is null) return true;
            if (left is null) return false;
            if (right is null) return true;
            return left.CompareTo(right) >= 0;
        }

        public bool Equals(ComparableMixin? other) // IEquatable<ComparableMixin>.Equals
        {
            if (other is null) return false;
            return this.EqualsImpl(other);
        }

        public int CompareTo(ComparableMixin? other) // IComparable<ComparableMixin>.CompareTo
        {
            if (other is null) return 1;
            return this.CompareImpl(other);
        }

        public abstract bool EqualsImpl(ComparableMixin other);

        public abstract int CompareImpl(ComparableMixin other);

        public override bool Equals(object? obj) // C# complains if we implement IEquatable without overriding Object.Equals
        {
            if (obj is ComparableMixin other)
            {
                return this.Equals(other: other);
            }
            else
            {
                return false;
            }
        }

        public override int GetHashCode() // C# complains if we override Object.Equals without overriding Object.GetHashCode
        {
            var hash = this.GetHashCodeImpl();
            return hash;
        }

        public abstract int GetHashCodeImpl();
    }

Consuming Type:
    [Mixin.Mixin(typeof(ComparableMixin))]
    public partial class Test
    {
        public partial bool EqualsImpl(Test other) => false;

        public partial int CompareImpl(Test other) => -1;

        public partial int GetHashCodeImpl() => 1;
    }

Generated code:
namespace ConsoleApp11
{
    public partial class Test : global::System.IComparable<ConsoleApp11.Test>, global::System.IEquatable<ConsoleApp11.Test>
    {
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator ==(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return true;
            if (left is null || right is null)
                return false;
            return left.Equals(other: right); // Invoke IComparable<T>.Equals, not Object.Equals, btw
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator !=(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return false;
            if (left is null || right is null)
                return true;
            return !left.Equals(other: right);
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator <(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return false;
            if (left is null)
                return true;
            if (right is null)
                return false;
            return left.CompareTo(right) < 0;
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator <=(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return true;
            if (left is null)
                return true;
            return left.CompareTo(right) <= 0;
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator>(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return false;
            if (left is null)
                return false;
            if (right is null)
                return true;
            return left.CompareTo(right) > 0;
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public static bool operator >=(ConsoleApp11.Test left, ConsoleApp11.Test right)
        {
            if (left is null && right is null)
                return true;
            if (left is null)
                return false;
            if (right is null)
                return true;
            return left.CompareTo(right) >= 0;
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public bool Equals(ConsoleApp11.Test? other) // IEquatable<ComparableMixin>.Equals
        {
            if (other is null)
                return false;
            return this.EqualsImpl(other);
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public int CompareTo(ConsoleApp11.Test? other) // IComparable<ComparableMixin>.CompareTo
        {
            if (other is null)
                return 1;
            return this.CompareImpl(other);
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public partial bool EqualsImpl(ConsoleApp11.Test other);
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public partial int CompareImpl(ConsoleApp11.Test other);
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public override bool Equals(object? obj) // C# complains if we implement IEquatable without overriding Object.Equals
        {
            if (obj is ConsoleApp11.Test other)
            {
                return this.Equals(other: other);
            }
            else
            {
                return false;
            }
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public override int GetHashCode() // C# complains if we override Object.Equals without overriding Object.GetHashCode
        {
            var hash = this.GetHashCodeImpl();
            return hash;
        }

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Mixin Task", "<IP_ADDRESS>")]
        public partial int GetHashCodeImpl();
    }
}

I propose the following (see Code Sample)  approach for replacement of other mixins.
I would restrict replacement to other mixinx because I currently do not see a benefit to replace other types. If non Mixin replacement is required, then I assume Generic parameters would be enough.

If you use the ImplementMixinAttribute the referenced type must be annotated with the MixinAttribute
If you use a mixin somewhere in another mixin it must be replaced, otherwise an error is generated describing the problem
The replacement must "implement" the corresponding mixin
A Mixin should (warning?) be Abstract, so it can't be instantiated. (I would like to also make it seald, but c# dosen't like that)

I see following drawbacks in this approach

Only Mixins can be replaced. So types that are from some librarys that propably do not use mixins can't be replaced.
The most prominent example of a type that is not a mixin that we want to repalce are numerics.
Every mixin can only be replaced with one implementation. I can't have two parameters of the same mixin type
that should be replaced with different consuming types.

This could be mitigated by defining a new type that derive from the desiered mixin and does not have any
implementation of its own

This will also not handle other kinds of replacements like constant replacement (string, int, etc...) this could be done by a different mechanic.
Interesting would be which use cases will work and wich will not.

Code sample
//---------------------------------------
// Attribute definition
//---------------------------------------

    [System.AttributeUsage(System.AttributeTargets.Class | System.AttributeTargets.Struct, Inherited = false, AllowMultiple = true)]
    sealed class ImplementMixinAttribute : Attribute
    {
        // This is a positional argument
        public ImplementMixinAttribute(Type mixin)
        {
        }

    }

    [System.AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, Inherited = false, AllowMultiple = true)]
    sealed class MixinParameterAttribute : Attribute
    {

        // This is a positional argument
        public MixinParameterAttribute(Type mixin, Type replacement)
        {
        }

        public Type TargetMixin { get; set; }

    }

    [System.AttributeUsage(System.AttributeTargets.Class | System.AttributeTargets.Struct, Inherited = false, AllowMultiple = true)]
    sealed class MixinAttribute : Attribute
    {
        // This is a positional argument
        public MixinAttribute()
        {
        }

    }

//---------------------------------------
// Mixins definition
//---------------------------------------

    [Mixin]
    public abstract class FooMixin
    {
        private readonly BarMixin bar;

        public FooMixin(BarMixin bar)
        {
            this.bar = bar;
        }

        public override string ToString()
        {
            return $"Bar is {bar.Name}";
        }
    }

    [Mixin]
    public abstract class BarMixin
    {
        public string Name { get; }

        public BarMixin(string name)
        {
            this.Name = name;
        }
    }

//---------------------------------------
// ussage
//---------------------------------------

    [ImplementMixin(typeof(FooMixin))]
    [MixinParameter(typeof(BarMixin), typeof(BarConsumer))]
    // ^^^ If this is missing, a compiletime error is generated 'cause we know that BarMixin is a Mixin
    // ^^^ and we do not know what to do with it
    public partial class FooConsumer
    {

    }

    [ImplementMixin(typeof(BarMixin))]
    public partial class BarConsumer
    {

    }

    public void Test()
    {
        var foo = new FooConsumer(new BarConsumer("Hello World"));
        Console.WriteLine(foo);
        // Prints:
        // Bar is Hello World
    }

//---------------------------------------
// generated Code
//---------------------------------------

    public partial class BarConsumer
    {
        public string Name { get; }

        public BarConsumer(string name)
        {
            this.Name = name;
        }
    }

    public partial class FooConsumer
    {
        private readonly BarConsumer bar;

        public FooConsumer(BarConsumer bar)
        {
            this.bar = bar;
        }

        public override string ToString()
        {
            return $"Bar is {bar.Name}";
        }
    }


[SECURITY] Nginx version is showing

When I got to: https://web.libera.chat/checkyourversion
I can see your Nginx version (=1.18.0). This is a security thread.
Please, try to disable powered by message and disable the server tokens, so you don't see the nginx version:
server_tokens off;
fastcgi_hide_header X-Powered-By;

Regards,
Melroy

Showing the version is only a security issue if we don't install security updates. If it were vulnerable to a known exploit and a fix would be available by updating, hiding the nginx version does not fix that vulnerability.

True, but it's still not wise to show the version. Because of zero-day vulnerability.
But well, you don't need to listen to me.

The very definition of a zero-day vulnerability is one in which there is no software update from the vendor for. I'm not sure what that has to do with whether the version is shown or not; by definition, all versions are vulnerable.

Because I can see whether or not you applied the patch already on a zero-day vulnerability.

Well I mean, if you can't see the version you'll just try the exploit anyways and find out that way if we patched it or not

That is also true. But you make it easier. That's it.
Should I close the issue? Since it seems that you don't want to change the nginx settings.


Suggestion & minimal implementation: Vite build

Hi Alon, it seemed a bit strange to me that you've decided to write your own bundling system instead of utilizing npm/yarn/pnpm (I went with pnpm in this pull request) with vite, the bundling tool introduced and maintained by vue team, so I decided to build this minimal example so that it's easier for you to evaluate it makes sense for you to use it in the future.
! I have not tested it with a running backend.
in web/app/:
dev environment with hmr:
pnpm install
pnpm run dev

production build:
pnpm install
pnpm run build

This may also be extended with following improvements:

version locking (pnpn-lock.yaml in this case)
TypeScript (tsc)

Where data types are also often provided from the community, e. g.

https://www.npmjs.com/package/@types/codemirror

https://www.npmjs.com/package/@types/d3

ESLint
...

Hey @qq7, thanks for that!
My initial rationale was to keep it simple, and not require build dependencies. But, the web/app has grown since its inception so it might be time to reconsider.


Test

https://github.com/DFE-Digital/teacher-cpd-design-history/assets/128088/77ecc2e8-7f66-4a21-87a9-f23ca645203e
https://github.com/DFE-Digital/teacher-cpd-design-history/assets/128088/8c45f7c2-9aad-49e1-96c3-f1b235c75680
https://github.com/DFE-Digital/teacher-cpd-design-history/assets/128088/5deab459-06c3-4c97-a1a0-4c2965610e80
https://github.com/DFE-Digital/teacher-cpd-design-history/assets/128088/1990f34c-7260-4433-828c-6a9723ee272a
https://github.com/DFE-Digital/teacher-cpd-design-history/assets/128088/41546a15-dd26-47a2-938b-df3680faf6d3


timestream-write: add support for customer-defined partition keys

What problem are you facing?
In June 2023, aws introduced in timestream the support for customer-defined partition keys.
aws blog post: https://aws.amazon.com/about-aws/whats-new/2023/06/amazon-timestream-customer-defined-partition-keys/
aws documentation: https://docs.aws.amazon.com/timestream/latest/developerguide/customer-defined-partition-keys-getting-started.html
terraform documentation: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/timestreamwrite_table#customer-defined-partition-key
How could Official AWS Provider help solve your problem?
Implement proper support for timestream customer-defined partition keys.

Looking at the terraform provider docs, this property isn't present in terraform-provider-aws version 4.67.0, which is what this provider currently uses. There are some breaking changes in version 5.0, that we are planning on upgrading to, although the details are still being worked out by some of the upbound folks.
Since this is just a schema change, we should get this "for free" once we're using the latest version of the terraform provider.

Currently available in the main branch, you will be able to use it next release. So I'm closing this issue, feel free to open it again if you encounter any problems.


Fix spelling errors

Simple spelling fixes.

Hi! Thanks for contributing. Nice find. Your fix is right, but not in the right file. webcontent.h is a generated file during the build.
Please modify the source index.html in the html folder. Thx!

Updated to do it properly.  Note, I removed some code in pre_build.py and pre_build_web.py, and ensured the webcontent.h file is created the same way by ordering the file processing in the latter, as os.walk() does not guarantee order, and when I recreated the .h file, the order was completely different, and by using sort the contents were generated in the same order.

Thanks! Ordering is okay, but unnecessary since it has no impact on the build process. It just minifies the file content. However you are right, if all builds are the same, it is better. :)

Updated to do it properly. Note, I removed some code in pre_build.py and pre_build_web.py, and ensured the webcontent.h file is created the same way by ordering the file processing in the latter, as os.walk() does not guarantee order, and when I recreated the .h file, the order was completely different, and by using sort the contents were generated in the same order.


Place concourse ctrl script in /etc/init.d so it can be managed by "service" command if OS == Ubuntu/Debian

Right now the script is place in /usr/local/bin, which is cool, but a symlink should also be added to /etc/init.d. If we do add this symlink then we must be sure to remove it when the uninstall script is invoked.

Okay so I went into the  /usr/local/bin via the terminal. There is a concourse file in there. But I can't seem to find the /etc/init.d path. How do I get in there? after i get in there do I: ln -s /usr/local/bin concourse?
Date: Mon, 19 Oct 2015 22:49:06 -0700
From<EMAIL_ADDRESS>To<EMAIL_ADDRESS>Subject: [concourse] Place concourse ctrl script in /etc/init.d so it can be managed by "service" command if OS == Ubuntu/Debian (#87)
Right now the script is place in /usr/local/bin, which is cool, but a symlink should also be added to /etc/init.d. If we do add this symlink then we must be sure to remove it when the uninstall script is invoked.
—
Reply to this email directly or view it on GitHub.


Dart VM using much more memory than Observatory reports

Hello! I have been doing thorough investigations into possible memory leaks in our own codebase for DSA. However, I am noticing that the memory usages displayed in Observatory are massively off from what I am seeing in System Monitoring tools. On the reproduction below, I'm getting hundreds of megabytes more in real memory usage than what I see in Observatory.
I have created a special repository for storing reproductions of these kinds of things. Sorry for not including all the info here, if that is desired rather than externally linking it, please let me know.
Reproduction Information

Nice work

May be related to #25178 and #24613

Not related to #25178 or #24613 as in this report the observatory is not showing where the memory is being consumed unlike in these other issues.

cc @johnmccutchan @zanderso

Has it been reproduced yet? If it is reproduced by others, it could be a potential blocker for 1.14.

@kaendfinger I have been running your reproduction on Linux with 1.14-dev.7.2 for two hours and I haven't seen anything yet.
Virtual Memory: 705.9 MB
Resident Memory: 175.7 MB
Writable Memory: 169.8 MB

@johnmccutchan It looks like this might have been the result of accidentally using the wrong Observatory for another test I was doing, which resulted in the memory usage being higher.
I'm going to close this issue, the memory leak investigation for DSA has lead us to find a leak in our own code. Thanks for the help :)


Writing best_k_models.json after every epoch

Hi!
First, I would like to thank you for providing such a great tool for speaker separation research. I love it!
However, I have a slight suggestion. When using the recipes from your egs directory, the best_k_models.json file gets written only after the whole training is finished. This way you cannot just stop your training earlier and go to the evaluation stage because the json file is needed for it. I suggest modifying the code so that the json file is dumped after the 1st epoch, and then updated after each epoch so that you can interrupt your training and go straight to the evaluation stage.
Cheers
Peter

Hey Peter,
Thank you very much for your kind words !
Thanks for the suggestion. Do you already have a simple way to do that?
I don't think we have access to the checkpoint callback through System so that's not directly obvious to do. I can see several ways to do what you'd like but none of them is the obvious one.
Would you like to give it a try? We could discuss your approach first

@masztalskipiotr, want to give it a try?

There are comments regarding this proposition in #96, just in case.

A way also to do it is to have an optional checkpoint argument in eval.py with name of checkpoint.

Closing this, feel free to re-open


Spot Light Example Update

Fix #229
TODO

Make it look nice
Clean up code
Fix robot model textures
comments
add comment-able list of spotlights in the back to show profiles / debug

cc @richardassar I updated the spotlight demo to use a more interesting model - let me know what you think!

Oooh, very nice! I like it!

https://gkjohnson.github.io/three-gpu-pathtracer/example/bundle/spotLights.html
😁