Proposal: Commutative operator overloading

When I first saw #746, I thought about Python.
In short, that issue talks about commutative operator overloading; that is, the ability to overload not just <myclass> + <otherclass> but also <otherclass + myclass>.
This is intuitive to anyone for numerous operators: +, *, etc. on numeric types and more (e.g. if I would want to implement a complex number class in Wren).
Unfortunately, the strategy used in Wren for operator overloading is simple: check if the left operand has a method named same as the operator, and call it with the right operand. This makes it impossible to overload operators commutatively.
Python solved this in two steps:

Define a constant NotImplemented, which any overloaded binary operator should return when there is type mismatch; in the example of the above issue, Num does not know how to multiply itself with List, so it'll return NotImplemented.
Define a reverse operator, __r<operator>__, for each binary operator. If the method of the left operand returned NotImplemented and there is a reverse operator method on the right operand call it with the left operand as parameter. If it returned NotImplemented too, raise an error.

Wren can implement a similar strategy too. We can define a static method Object.notImplemented. About the second step, I have two ideas:

Just like Python, add reverse operator semantics. Something like a reverse keyword, e.g. reverse *(other). This has the disadvantage of adding a keyword and requiring another method for the same operator (usually it'll just return this * other).
Define a specific set of operators as commutative, and automatically call them as reverse operators if needed. I do not like this approach, because although most of the time there's a constant set of commutative operators, there can still be cases where I want to overload both the reverse and the straight version of the operator. For example, if I'll continue with the complex numbers example from above, I'll want to be able to do both complex / irrational and irrational / complex, even though they have different meanings.

This is a huge can of worm, with no real easy solution. The solution you
propose is valid, and try to solve the not having control of the source.
Another solution to this problem could be to allow to expand classes
outside of primary class definition, or granting an operator to be
commutative on some class.

@mhermier I liked the second solution, but I think the first breaks the nature of Wren. It'll also make it much less performant.

Elaborate how it can be slow?
If you craft an opcode to swap arguments on the stack, and call the
operator again (and given it can be re-used per operator, I don't see how
it can be a penalty compared to do a full round.
First option implies, trying to do a call, asking querying the class, and
then perform the same evaluation as on previous definition.

I don't understand. Do you mean the following?
class Complex {
  construct fromNum() { /* ... */ }

  *(other) {
    if (other is Num) {
      return this * Complex.fromNum(other)
    }
    // ...
  }
  multiplyNum_(other) {
    // `this` is `Num`
    if (other is Complex) {
      return other * this
    }
    return Num.originalNumMultiply_(other)
}
Num.originalNumMultiply_ = Num.*
Num.* = Complex.multiplyNum_

In essence yes, maybe not exactly that syntax. Or something like this:
class Complex {
}

Complex.* = Fn.new {|lhs rhs|...}

But details have to be thought, because this involves overload name
resolution which is another can of worms that need to be think of...

I thought about block functions, but rejected it because they requires .call().
Except the boilerplate (which is huge, as you can see in my example) and the compromise of readability (again), this will make the VM slower because the speed of Wren comes mostly from the speed of method/fields location - they're known at compile-time. If we'll be able to change existing class structure (like other dynamic languages), we'll loss this advantage. Field and method access will need to perform a lookup in a dictionary, and it'll slower the VM in factors (I can guess more than 2x).

Behind the scene it is a linear search, so it is only a matter of
expanding an existing class, so speeds as this is not really an issue
(as in current implementation). The only real issue is control where we
allow expanding structs, so we don't get some oddities/corruption.
About the .call() issue, the argument is not a real issue. Since
methods/foreign are not first class object (yet), it can state that they
follow .call() protocol

Behind the scene it is a linear search, so it is only a matter of expanding an existing class, so speeds as this is not really an issue (as in current implementation).

Linear search? What do you mean?

The only real issue is control where we allow expanding structs, so we don't get some oddities/corruption.

structs in Wren?

About the .call() issue, the argument is not a real issue. Since methods/foreign are not first class object (yet), it can state that they follow .call() protocol

If they were first-class they could implement a protocol, but now when they're not?!
Can you not put too many newlines please? It is hard to read...

Classes holds methods in a linear and predictable way in the current
implementation. Meaning if you define a method name foo(), it will get
an index assigned, that will be available forever and every class during
the lifetime of the VM. So expanding an old class to provide foo(), is
only expanding the array of methods array so it can contains the index
for foo() and assign the method at it.

Yes. But the index is provided at compile time, and with that it'll have to be generated at run time. See:
class A {
  f() {
    System.print(this.f2()) // `this` is required, or the compiler won't know where `f2()` is defined
  }
}
A.f2 = A.f // Of course infinite recursion, just for demonstration

When compiling, the compiler won't know how to access A.f2(), so it'll need to leave it as-is. And the interpreter will have to perform a string lookup.

And also this will be asymmetric and confusing, since it won't allow getters/setters/static functions.
Also the compiler will need to accept operators as class methods Class.*. Minor change, though.

There is no such error, because the compiler is dumb. Whenever it finds
and unknown methods, it creates an entry. So as in current state, it
should be valid given method assignment works.

Also, now this outside of a method is a compile-time error. It will have to be a run-time error.

There is no such error, because the compiler is dumb. Whenever it finds and unknown methods, it creates an entry. So as in current state, it should be valid given method assignment works.

OK, although that seems to be a bug in the compiler (accessing an undefined method will not raise an error).

This starts to be really be out of control, adding too much work around.
It make the function approach less intrusive.

There is no such error, because the compiler is dumb. Whenever it finds and unknown methods, it creates an entry. So as in current state, it should be valid given method assignment works.

But there is still problem - consider:
class C {
}
class Helper {
  f1() {
    this.f2()
  }
  f2() {
  }
}
C.f1 = Helper.f1
C.f2 = Helper.f2

It would be a compiler error, if a more advance type checking was in
place. Currently type checking is in-existent at compile time, and is
only done at runtime.

This starts to be really be out of control, adding too much work around.

I absolutely agree. As I stated above, this is a complete change to the design of the language. Massive changes to the language requires massive changes to the implementation, naturally. This is the reason I rejected this idea.

It would be a compiler error, if a more advance type checking was in place. Currently type checking is in-existent at compile time, and is only done at runtime.

Unfortunately, you're right. This is an inherent limitation of single-phase compilers.

Your example is demonstrates why methods should not be shared between
classes. And mostly make it only valid to use function instead.

But functions should have .call() as I said, and also suffer from this issue:
class C {
}
C.f1 = Fn.new {
  this.f2()
}
C.f2 = Fn.new {
}

I also suspect there are bunch of issue we haven't discovered. It's better to avoid this idea completely, especially since there are excellent alternatives.

No, this is an abuse/dirty hack. You have to consider this as the first argument of the function, like what you would do with method introspection, and you are done.

And what about .call()? And the issue in my example?

And where is the dirty hack here?

Example my way, that don't require a grammar hack so this is the real object not the method:
class C {
}
C.f1 = Fn.new {|self|
  self.f2()
}
C.f2 = Fn.new {
}

OK. But I think this is confusing and we all better with the previous ideas. Your thoughts?

My thought is that has I previously stated, this is an expected mess. And your syntax, with this in the function, is pretty confusing, because this could also represent the function object itself. Introducing a new syntax for free methods could also be a solution. But each of the solutions seems to have pretty huge cons, mostly because it requires type inference.

I'm talking about adding a reverse or reversable keyword, to indicate reverse operator or operator that can be applied in reverse, accordingly.

Grammatically wise it seems to be the less intrusive, thought it might
requires to adapt a bunch of code to detect the rhs argument type. It also
means that the reverse call is opt-in by the original caller, which can
also be a minor problem in 3rd party code where maintainer is not willing
to be compliant.
Implementation wise it might be a mess. Either you return the error by
return value or by exception, first implementation being a little bit less
worse than the other, it will introduce a conditional jmp (and creation of
fibers in the second case) in assembly on every potential operator call
(same problem with the assign operator proposal). Additionally this would
require a new singleton so it can be used inside the VM or that we
introduce VM error enum and its bindings.

Side note about the new lines: I'm sorry, but I was replying over emails on the phone, and it seems that gmail or github add spurious new lines when adding comments.

Do you have better suggestions?

Not really, I'm just saying, before implementation that it will have quite some bytecode cost, even creating a dedicated opcode for it will have possibly high cost.
The thing is that problem you are trying to solve is a common but artificial problem caused by code distribution. Since all the source must be feed, you have full control of what you feed and can correct broken operator/code.

But you agree that the current state isn't good.

It is not good or bad. For my use case, I consider that I have access to
all the code, and I can fix any incorrect behaviour already. For your use
case, where you want to use external code, this is a requirement if you
don't want to alter external code. But what ideally I really would like to
avoid is to pay for feature that might be barely used.

I meant nearly used in a hot path. There are change that can be considered
good while heavy, if in the cold path. The problem with that change and the
assign operator changes is that it will end up in a hot path at 99% of use
cases.

The biggest problem here is the inconsistency. A beginner can spend a lot of hours trying to understand why [1] * 3 is [1, 1, 1] while 3 * [1] is an error. This is not what users expect from their language.

Unfortunately you're right about the downgrade in the performance. I will think about it a bit more trying to find a solution, but one way or the other I think we should implement it. Performance is the second most important feature of a language, but first and foremost we have the language itself - its semantics, syntax and what users want it to be.

I thought about it a little bit more, and I think you're wrong. This seems to be on the hot path, but it's definitely not.
See: the only impact of this will be in the case of invalid operator. That is, if we do 3 * 3 the VM will immediately call Num.*(3, 3) and everything is fine.
The problem starts where we do 3 * [1]. Previously that was an error (the Num.*() method detects the use of a non-number as the right operand and call Fiber.abort()). Now it'll call List.*([1], 3). Clearly, errors are a very cold path. Many languages use this fact: V8, AFAIK re-parse the source code with location information (row and column) where there's need to get a stack trace, and so it can skip this step in the first parsing, saving memory and computing power in the regular case. Take this as an example. Instead of emitting an error immediately, we'll have a delay. But it's definitely worth it.
Also, we will gain a small (or not-so-small) benefit: clearer error messages. Don't look at me like that, error messages are very important! Look at this small example:
class C {
  construct new(f) {
    _f = f
  }
  f {
    return _f
  }
  *(other) {
    return C.new(f * other.f)
}

var c = C.new(5)
System.print(c * 5) // Cryptic error message: "Num does not implement 'f'". Clarification, please?

Of course we can check for non-C parameter in C.*(), but we won't. This is how users write programs.
Now they'll have to:
class C {
  construct new(f) {
    _f = f
  }
  f {
    return _f
  }
  *(other) {
    if (!(other is C)) {
      return System.notImplemented
    }
    return C.new(f * other.f)
}

var c = C.new(5)
System.print(c * 5) // "Unsupported operand types for '*': 'Num' and 'C'"

About your first arguments, it is true that it can be disappointing. But
there only 3 ways to fix it:

You fix it for yourself.
You can fix it and you propose it to the maintainer.
You fix it by using a language feature so you don't have to ask the
maintainer of other code.
The exact example you are giving is an example of what should be in the
second way. But what you are trying to do is to push the 3rd way, by
bringing a language feature.
Each of these way have interests, and 1st and 2nd usually goes in pair
usually you do the 1st and when you are confident you do 2nd.
I agree the 3rd way has various interest. But it is not also a universal
solution, like subclassing/separation...
About the performance degradation, I considered even creating a specific
opcode, it can mitigate the problem by joing call and conditional jump in
one opcode (or even fully integrate it in one opcode). But at the end of
the day it is still a band aid, and just optimise the issue, not really
solve it. Maybe that one can be tested, but I don't really have high hopes
because of the complexity it brings to do the error management, and you
also have to garantee there are no abuse of the error or unexpected
propagation of that error and provoke erroneous behaviour. I don't say it
can be worth the effort, but it has to be tested and validated.

You are right, error management should go in the cold path, but how do you
handle the error in the hot path? You must have to alter the hot path in a
way that it is prepared to receive the error. And this is where lies the
issue. There are some cheaper way to do it, but nothing is cheaper than
having nothing to do. In static language this cost is lifted at compile
time, in dynamic language this cost is resolved at runtime and can become
really expensive.

This way already exists. Built-in classes check their operand. For others, the impact is not so great.

For your specific issue, you can do it the same way, without the need to
introduce the heavy machinery required by what you originally proposed.
This machinery is useful but you have to introduce it with care and
awareness.

I have no specific issue. I've encountered #746 accidentally. And I think that just like there the user got confused from this behavior, many will too.

A reason way I'm a bit reluctant about the implementation you proposed,
consider the following code:
for (mat in matrices) {
  norm_mat.append(mat.norm() * mat)
}

Your assumption that it is a slow path is thrown away, and if you have
gazillion of matrices, the cold path is the hot path.
I agree this code is a little bit artificial, but you have to consider it,
in its generality, and the probability that the cold path become hot is
very likely if generalised...

Where are the performance hits in your example?

mat.norm() * mat is the only operator. I agree that my example might be poorly chosen because of the matrix allocation that might be the slowest part.
But lets consider implementation of reverse operator using exception. To perform it, the following logic has to be done (in pseudo code):
{
  fiber = Fiber.new()
  push result of mat.norm() on top of fiber
  push mat on top of fiber
  fiber.try("operator")
  if (!fiber.hasError() || fiber.error != NotImplemented) return fiber.value in current fiber
  swap arguments on current fiber
  call("reverse operator")
}

This pseudo code already optimize second call by using current fiber. But the first fiber creation on hot path is almost a requirement, unless you use some heavy hackery on the current fiber.
Now imagine that you have to do this on every binary operator in existence... because you'll never know when a reverse operator will occur...
Even if you can reduce/remove some of the code by doing a dedicated opcode and hackery, there is a bunch of extra logic to be added in the hot path.

You don't have to raise an error. You can just return value. Then your code become:
{
  push result of mat.norm()
  push mat
  call("operator")
  if (stack top == NotImplemented)
  {
    pop
    call("reverse operator")
    if (stack top == NotImplemented) Fiber.abort("Unsupported operand types for '*': 'Mat' and 'Mat'")
  }
}

Which is essentially two more CPU instructions for the hot path (cmp stack_top, NotImplemented and jnz after_if) in x86. Probably ARM will have three because we'll have to calculate the stack top, but I'm not sure. Anyway, this is not a big deal.

Well due to how stack works, there is more stack manipulation than that to do. It is a little bit worse than that, since return will remove the first pushes, you have to dup the stack values and restore the stack to propagate the result in the hot path, in cold path you only need to pop and swap the arguments. These are quite cheap in a single usage, I agree. But consider even a single matrix manipulation or statistics, and also the damage this change does to regular math when cumulated, with all the preamble added to the hot path ???

Hacky, maybe, but a single DUP_2_TOPMOST bytecode instruction will solve the problem. And as I said, I think it's mandatory: first and foremost the users, and just then the performance.

Test it and give numbers. It will show usability, and will give some reference numbers to prove or not my point. And no, nothing is ever mandatory (other than the foundations required for computation), everything (else) is a trade of. Some of my previous patches were refused while resolving the stack corruption issue for good, with marginal effects on the speed (~15% speed decrease). And they where refused because of these marginal effects even if stability is mandatory.

I gave a quick try at implementing it, and found a new corner case. Consider:
class Foo {
  construct new() {}
}
class Bar {
  construct new() {}
  reverse +(foo) { 42 }
}
Foo.new() + Bar.new()

In that case method will not be able to be resolved, as such if normal evaluation takes place it will throw an exception. Issue can be mitigated by checking for method presence, but that means additional checks in the hot path.

I'm working on implementing it, and have an (almost) working solution that bypass this obstacle too. Hope to send a draft PR for few days.

I have not benchmarked yet, but from my calculations I except max. 5-6 CPU instructions on the hot path. That means completely opaque effect.

I made an attempt, because it can be done by adding only one opcode but
requires some refactoring of calling methods.

Exactly what I did 😃

Ah, I added two. One for calling binary operators and one for returning from them.

You can do it all at once, by ensuring the 2 operators are in sequence (or
passing 2 method index, but it doesn't add much). Since the rest is fixed,
this can really be BINOP_CALL dealing all the mess returning result of top
of the stack.

But when will I call the reverse operator?

Since it is inside the VM, the opcode can do anything and you only need to
add 1 to get the reverse operator (require a convention). But it need
internal rework, to avoid code repetition.

The symbol is same. But I need to call the reverse operator it when it returns, not when it called.

The symbol (as symbol id) can't be same, otherwise you end up
disallowing/overwriting normal operator.

I think we don't understand each other try to do something, if I can do
something I'll try to, but thing can be messy because of the pile of local
changes I have.

The symbol is generated by name. Every * method, for example, will have the same symbol across the same module.

And whilst foreigns and primitives returns immediately, for user methods we need to wait until the CODE_RETURN before we know the return value.

I went with a reversible keyword. However, after I had a complete implementation I've noticed a glitch. Operators can be commutative, but class cannot overload operator with different meanings. Consider the following code from the standard library:
class List is Sequence {
  +(other) {
    var result = this[0..-1]
    for (element in other) {
      result.add(element)
    }
    return result
  }
}

Now, I want to be able to write both [1, 2, 3] + (4..6) and (1..3) + [4, 5, 6]. But if I mark the operator as reversible, the second expression gives [4, 5, 6, 1, 2, 3].
Here are the solutions I've thought about. I'd like to hear you feedback:

Be able to mark another method as reverse. I still don't know if I want to remove reversible or leave it (it is useful in many operators, but reverse can cover that, although with a bit more code). This is my preferred option.
Make binary operators take optional argument isReverse which would indicate whether this is a straight or reverse call.
A really breaking change, inspired by C#, and partly C++: make operators static methods. Then, they'll receive two arguments, lhs and rhs. I don't like this approach, because it'll: 1) be a big breaking change. 2) require a lot of boilerplate code to see where's the class instance. 3) make operators can't access fields.

Can you show the reversible usage? Because your examples is a bit confusing.
Problem you are seeing is that you are trying to augment an operator,
thinking that it is commutative while it's not...

No it's not. But this is still natural for users to do (1..3) + [4, 5, 6]. An example of a reversible operator is the List.* operator in the standard library (there is also String.* which is very similar):
class List {
  reversible *(count) {
    if (!(count is Num)) return System.notImplemented
    if (!count.isInteger || count < 0) {
      Fiber.abort("Count must be a non-negative integer.")
    }

    var result = []
    for (i in 0...count) {
      result.addAll(this)
    }
    return result
  }
}

Updated the title to reflect that.

I wonder if your confusion does not come from the fact you think that a
class operator should be reversible or not. For me one should be able to
provide both. I mean, commutativity depends on both operands, so it might
be desirable to provide something in the regular path and something else in
the reversed path to solve ambiguities.

This is exactly what I figured some hours ago, and I ask your advice.

I know it since almost the beginning, and I think it is best to have the 2
definitions. Example:
class Foo {
  *(rhs) {...}
  reversed *(rhs) {...}
}

That way you control both call path separately. Reversed meaning that the
regular call failed, and it is not an ambiguous situation where the
priority start a guessing game.
There can be some issues calling regular and reversed from each other, but
it can be dealt with an external function till things get sorted.
Now that I think again at it, if the reversed method is missing it will
trigger an exception... So there is also a mess of error handling to
consider/resolve...

If so, why did you misled me and proposed a way to mark same operator as reverse? 😠
I said,

Just like Python, add reverse operator semantics. Something like a reverse keyword, e.g. reverse *(other). This has the disadvantage of adding a keyword and requiring another method for the same operator (usually it'll just return this * other).

And you responded,

Another solution to this problem could be to allow to expand classes
outside of primary class definition, or granting an operator to be
commutative on some class.

And believe me, yes it was hard, but I did that 😃

And I'd still like to hear your opinion about having both reversible and reverse keyword.

It is an error in communication. What I meant that the commutative operation could be accessible on some class. I didn't explicited the way to do so. Back then in my head it was trivial that granting was an addition, not a modification of the current behavior.
To me the keyword is not yet important, it is still just a marker to test the feature. Ideally you should do the search for previous art in other languages on that feature, justify your choice based around prior art and what is different/related to other languages that explain your choice, and hear about other testimonies. You already talk about python I think, but if you could look in some other languages for reference (if there is any) it would be nice. Example C++ handle the situation by allowing external overloading, which is not really applicable in Wren lang. If a consensus emerge you have your choice in the syntax.
For me this feature is quite a novelty, thought I remember about some formal languages allowing commutativity definition. But most of them are statically typed so there is no prior art I can really think of, that would (in)validate using a particular syntax, so I'm still open for ideas. That said, the keyword approach is quite good, thought it add some crust to name resolution for named call.

OK. C++ and C# offer similar solution, by not always passing the receiver as the first argument (either by method being static or by not being method at all).
Python strategy, converted to Wren, will mean adding a reverse keyword,
Those are the languages I know that have operator overloading. You're more than welcome to add more. In the meantime, I will change my code from reversible keyword to reverse keyword (it's not so much work - the VM is almost the same; only the compiler and the core library will need more than trivial changes). I hope to open a PR tonight (I'm UTC+03:00).

Do you think we need to make having only the reverse operator an error?

I would say the situation is complex and it has to be seen in action to take proper decisions. Problem with error is that you can intercept ones from lower in the call-stack and misinterpret situation. With return value it is a little bit harder but it can also append. So lets have something to toy with and see how hard we hit the wall.

Ahm, I'm researching the area now, and Ruby behave the same as Wren. I can understand why 😢

OK. Here's a summary of various programming languages and what they do:

Lua:

Arithmetic operators (+, -, *, /, %, ^, .. (concatenation)) - if the left operand defines the operator, it wins. Otherwise, we look at the right. Operators can know the order of operands because they're passed by order (like static method, but they took a strange decision and made them instance method - so self is not the class type 😄).
Comparison operators (==, !=, <, <=, >, >=) - the operands must be of the same type and have the same operator method (I don't understand why both constraints are needed - isn't the second implied by the first?). This way, they eradicate the problem. I can't understand the reason behind the differentiation between arithmetic and comparison operators.

C++/C# - disconnect operators from instances, and rely on static typing. That is, to be able to overload the reverse version you're free to reorder the operands (no instance method), and to know where you can't handle a type you list all types you know how to handle (static typing). Wren can do the first, but not the second. A viable solution will be to return System.notImplemented in order to indicate unknown type (a combination of C++/C# and Python approaches).
Dart, Ruby - do not handle this problem. Overloading reverse operators is not possible (technically yes using monkey-patching (extension methods in Dart), but this is strongly discouraged).
Python - provides both straight and reverse operators (__r<op>__ magic methods) and relies on returning NotImplemented to know where to call the reverse operator. This is the best solution I've found, but it hardens commutative operators overloading and encourage code duplication, or at least boilerplate. Consider the following code I wrote from the core library of Wren:

class List {
  *(count) {
    if (!(count is Num)) return System.notImplemented
    if (count < 0 || !count.isInteger) Fiber.abort("Count must be a non-negative integer.")

    var result = []
    for (i in 0...count) {
      result.addAll(this)
    }
    return result
  }
  reverse *(count) { this * count }
}

class String {
  *(count) {
    if (!(count is Num)) return System.notImplemented
    if (count < 0 || !count.isInteger) Fiber.abort("Count must be a non-negative integer.")

    var result = ""
    for (i in 0...count) {
      result = result + this
    }
    return result
  }
  reverse *(count) { this * count }
}

[1, 2, 3] * "not a num";

This code does not work. It enters an infinite recursion. Can you see why? List.*(_) returns System.notImplemented, and the VM calls String.reverse *(_). It calls String.*(_) but here we enter a new series of binary operator calls, and so after String.*(_), returns System.notImplemented the VM calls List.reverse *(_). And so on. This is exactly what Python does in a similar case.
The solution is to not call the straight operator from the reverse one. But it's easy to forget, and even worse, this is a code duplication. We can obviously extract the code to a method, but then we have boilerplate. Either way we lose.
I think the best will be to have both reverse and reversible keywords. This way, commutative operators can be reversible while others can overload the reverse operator. I'm waiting for your feedback.

The Haxe language use the @:commutative decorator/meta-tag to specify the property, but it is helped by the fact that the language is statically typed.

I just thought about something trivial that needs to be checked, the operator that are involved as left value evaluation expression should be forbidden and checked by the test suite. So far, I think it should at least imply assignment syntax (this.foo = bar) and subscript operators (foo[bar]), but in the future it might also mean arithmetic assignment operator (foo += bar, foo -= bar, ...).

I've encountered a problem. Consider:
class C {
  ..(other) { }
  reverse ..(other) { this...(other) }
}

The programmer meant to call the straight .. operator on C, but the lexer emits the token TOKEN_DOTDOTDOT and the interprets the expression as this ... other. Same problem for calling ..., the parser will emit TOKEN_DOTDOTDOT then TOKEN_DOT instead of TOKEN_DOT then TOKEN_DOTDOTDOT required to correctly interpret the expression.
The problem can be solved by adding a space, but can you understand the following code?
class C {
  ..(other) { }
  reverse ..(other) { this. ..(other) }
}

I thought about allowing (requiring?) parentheses around directly called binary operators:
class C {
  ..(other) { }
  reverse ..(other) { this.(..)(other) }
}

This is better, but I still have few questions:

Do we need to allow them (but also allow separating using space) or require them around .. and ..., or maybe even require them around any operator?
Should we allow them around ordinary named methods (this.(m)()) to keep consistency, or not?
Maybe we should choose a completely different solution?

I'd like to hear your opinion.

And if we allow (or require) parens around every operator, what about reverse ones? Should it be this.reverse (+)() or `this.(reverse +)()?

I didn't considered the .. and ... operators in my proposition. But do we
really need to allow them to be reversed? Since the original definition is
pretty one sided (non reversible) I would not consider it a candidate to
the reverse operator.
If we really need to allow them, the space disambiguation is the simplest
solution and it cost absolutely nothing.
Personally I would be against using parenthesis there, I would reserve it
to method evaluation, to separate it from the usual name resolution.

I don't see a reason why not allowing reverse them. They are binary operators just like others. If anything, I wonder why to allow overloading them at all (Python does not allow overloading the colon operator used to create slices, for example. C# also does not allow it).
The problem with the space is that it's easily slipping out of sight. It's hard to distinguish between a. ..b and a...b. The only visual clue is the parens, because although a...(b) is valid, it's not common. But, for example, a...(b + 1) can be written (although a...b + 1 is the same, many people will prefer to be explicit, especially in complex expressions).

And (curiously): what's method evaluation? Do you mean dynamically choosing the name of the method (like object[property] in js)?

The parenthesis of parameters option is bad, it complexify the compiler,
that needs to elaborate on the argument. I  addition, there can be some
legitimate case where parenthesis are required.

Something more like c++ operator ->* where rhs is a computer method object.

But Wren does not allow you to read method object 😉

And can you give an example of case where parentheses are required please?

Why is it complicates the compiler? I mean, it does complicate things, but not so much.

It doesn't allow for now, but we should consider it as a possibility, a
don't shoot ourself in the foot by not looking in the future...
I don't have all the operator precedence in mind, but there is no guarantee
that the .. operator will have the smallest precedence, there will always
be possible ambiguous case were parenthesis could be required.
It complicated the compiler because technically while it can still be done
with an LL(1) language, I think it would make it an LL(2) or even LL(3).

No there are not such places. We're talking about wrapping the operator itself (a(..)b), not the operator with its operands ((a..b)).

I see but how would you implement method call (without mirroring since it
would fall in the call protocol)?

I don't understand the problem, sorry.

If you mean a(..)b is ambiguous with a(123), then you're right. This was just an example, but since we're talking about operator invocation there will be always a dot before: a.(...)b.

Given the fact that you could get/store a method object into a variable,
give a syntax to call that stored method on a given object.

But where is the problem here?

There is no real problem. I just find it not elegant at all, and gives ()
an usual meaning of special name resolution. I  contrary, I find
obj.(method)(params) (where all names are variables) more elegant, even
if it means that for 2 operators, it requires a space to disambiguate the
call of obj. ..(param).

OK.

Got it. Although we can (with LL(2) parser) have both.

Well there is always some possible ambiguity somewhere that I didn't think
off. But the more I think of the `a(..)b syntax the more ugly I found it

I personally don't agree. This is what F# has, and it's perfect. But I'll ignore my opinion 😉

You can "not agree" with me, in the contrary it is good. But bring some
counter arguments/solutions to the table. It is not because one language
does it, that it is an absolute solution. They could have been trapped by
the grammar, been lacking of imagination, been referencing an obscure
solution taken from what ever language....

F# has a single-phase compiler, two. Already one similarity 😄.
In fact, I brought solutions and you rejected them 😃.
I am not one of F# developers, and can only guess the reasons. But it seems reasonable that the limitations of single-phase compilers enforced that.
But we're both agree that both solutions are possible technically. All our controversy is which one is more readable.
And finally, as I already said, I accepted your opinion for the implementation. We're arguing, ahm, in the air.

I don't rejected them, I don't find it viable if you consider method
invocation.
If you have a solution to express method invocation, operator or what ever
you are welcome...

I think that the functionality to direct call function should be moved to
this own thread/pull request. I mean, even if I like the dot syntax to
extends, it is its own can of worms than what the original proposition was
about.

I don't agree since the only valid use case for this is for reverse operators. I even considered making other uses an error.


Add release notes section and content to the docs

Adds the release notes to the documentation and will be updated on each new point release.

Looking great! I've also noticed we should update our use of "catalogues" to "catalog" for American English


converting json to vector of type with templated constructor

Bug Report
The following code fails to compile when uncommenting line 16:
#include <nlohmann/json.hpp>

namespace nl = nlohmann;

struct S {
  S() = default;
  template<typename T> S(const T& t) {}
};

void from_json(const nl::json& j, S& s) {
}

int main() {
  nl::json j;
  auto s = j.get<S>();
  //auto v = j.get<std::vector<S>>();
};

The error I get is
json.hpp(1360): error C2338: could not find from_json() method in T's namespace

The cause seems to stem from the check for std::is_convertible<BasicJsonType, typename CompatibleArrayType::value_type>::value on void from_json(const BasicJsonType& j, CompatibleArrayType& arr).
Since you do not convert directly from json to the array value but call get() I think this check can be changed to allow this to compile.
Compiled on VS 15.6.0 preview 2.0
Library version 3.0.1

@theodelrieu Any idea on how to proceed with this issue?

IIRC this was related to #958, I'll check tomorrow the compiler errors to be sure.

With PR #969 and the code in https://github.com/nlohmann/json/issues/924#issue-289805274 compiles without issues.

Thanks!
On 12 Feb 2018 7:18 pm, "Niels Lohmann"<EMAIL_ADDRESS>wrote:
Closed #924 https://github.com/nlohmann/json/issues/924 via #969
https://github.com/nlohmann/json/pull/969.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nlohmann/json/issues/924#event-1469945802, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AHTojc8JqE6T1YAyM2JSuyrV1v3ls0Dxks5tUHJTgaJpZM4Rjs5L
.


Two approaches for outliers removal

I discover different approaches for outlier removal. Do we need to synchronize?

in current repo, till select HI and spike number, applied group_by(Year, Location, Treatment, BRISONr) and set criteria of 2 sd

## Filter Harvest Index by Standard Deviation
complete_dat <- complete_dat %>%
  group_by(Year, Location, Treatment, BRISONr) %>%
  mutate(HI_sd = sd(Harvest_Index, na.rm = TRUE),
         HI_mean = mean(Harvest_Index, na.rm = TRUE)) %>%
  ungroup() %>%
  mutate(Harvest_Index = ifelse(Harvest_Index < HI_mean - 2*HI_sd, NA, Harvest_Index),
         Harvest_Index = ifelse(Harvest_Index > HI_mean + 2*HI_sd, NA, Harvest_Index))

## Filter Spike Number by Standard Deviation
complete_dat <- complete_dat %>%
  group_by(Year, Location, Treatment, BRISONr) %>%
  mutate(Spike_number_sd = sd(Spike_number, na.rm = TRUE),
         Spike_number_mean = mean(Spike_number, na.rm = TRUE)) %>%
  ungroup() %>%
  mutate(Spike_number = ifelse(Spike_number < Spike_number_mean - 2*Spike_number_sd, NA, Spike_number),
         Spike_number = ifelse(Spike_number > Spike_number_mean + 2*Spike_number_sd, NA, Spike_number))

complete_dat <- complete_dat %>%
  mutate_all(~ifelse(is.nan(.), NA, .))

in my manuscript I use for each trait (except for disease traits) with criteria of 4 sd

outlier_value <-<PHONE_NUMBER>9

rm_outlier <- function(x){
  x_m <- mean(x, na.rm=TRUE)
  x_4sd <- 4*sd(x, na.rm=TRUE)
  x <- ifelse(x >x_m+x_4sd| x <x_m-x_4sd,
              outlier_value, x)
  return(x)
}

data <- data%>%
  group_by(trait) %>%
  # 100$ disease infection will be excluded from the rm_outlier()
  # therefore, these trait should be skipped.
  mutate(Trait2=case_when(!trait%in%c("Stripe_rust","Septoria","Powdery_mildew","tinfect",
                                      "Leaf_rust","DTR","Fusarium")~rm_outlier(Trait),
                          # trait other than diesease need to filter the outlier.
                          T~Trait)

data %>%filter(!Trait2==outlier_value)

Harvest index are very low and in combination with normal yield (yield with very low harvest index should be very low) this is not realistic. If it is more comfortable for you, set the threshold to 250.


Add instructions for setting up local development environment

This is of course quite specific and opinionated, but on the short term I think that's fine? This should at least make it a bit easier to quickly set up for people using conda.

Great, thanks @jorisvandenbossche. Yes this is certainly better than the previous installation instructions!


Normalizes node features so they use LDK NodeFeatures

Currently we were using a pretty hacky way of parsing LND features that only parsed the keysend feature (given is the only we care about atm).
This PR properly loads the feature vector returned by LND's GetNodeInfo and parses it so a complete NodeFeatures object can be constructed.
Also sets NodeInfo::features to NodeFeatures for consistency

Some manual testing to make sure the features are properly parsed will be appreciated (I'm looking at you @okjodom, AKA fast-merger lol)

I've opened a PR upstream to add the regular feature bit parsing. We should be able to simplify this if that gets merged (I have code locally for it).
I think we could squash this and go with the current approach for now.
For reference: https://github.com/lightningdevkit/rust-lightning/pull/2522

Fixed @carlaKC comments and squashed both commits.

LGTM!
If we're going to merge before figuring out why our_features is None let's open a bug issue so that we don't forget about it !

I'm trying to figure out this, will merge tomorrow otherwise

We may want to merge this and keep track of the fix, if it doesn't happen to be me doing something weird

SGTM - created #85 to keep track of it!


Add alacritty's default theme pre-0.13

Alacritty v0.13 changed the default colour scheme. So I thought I'd add v0.12 defaults as a config.

I'm not sure that I like the name 0_12, maybe alacritty_classic should be better?

I'm not sure that I like the name 0_12, maybe alacritty_classic should be better?

I used this name because it's a clearer demarkation. Besides, there's a non-zero chance that the default theme gets changed again in the future, and then it would become confusing.

nah, it won't change, unless both maintainers change.

anyway, it's up to @chrisduerr how to name the old variant.


After click login nothing happen

Ive tried uninstall and install again , same issue
New-Tab (1).webm

@omarfarache I assume you are using API Key method, correect? You should see a textbox at the bottom of your sidebar conversation window. Due to the width of your window, it gets truncated from the bottom. That's an issue that I wasn't aware of, so thanks for bringing it up and providing a video for the problem, it's useful.
Once logged in you can ask a free form prompt using that textbox or select code and right click to see the ChatGPT: * options in your context menu

@omarfarache I assume you are using API Key method, correect? You should see a textbox at the bottom of your sidebar conversation window. Due to the width of your window, it gets truncated from the bottom. That's an issue that I wasn't aware of, so thanks for bringing it up and providing a video for the problem, it's useful.
Once logged in you can ask a free form prompt using that textbox or select code and right click to see the ChatGPT: * options in your context menu

Thank you , resolved
just need drag the menu to the right

Here there could be a minor improvements where if you are using the api key method the login button is hidden

@zk-g thanks for the suggestion. This improvement is work in progress and to be released sometime next week. Stay tuned!


fixed "StartTree" typo

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@e98efcb). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4571   +/-   ##
=========================================
  Coverage          ?   64.59%
  Complexity        ?       20
=========================================
  Files             ?     1070
  Lines             ?    55596
  Branches          ?     8107
=========================================
  Hits              ?    35912
  Misses            ?    17027
  Partials          ?     2657

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e98efcb...11c5572. Read the comment docs.


Apparent race condition in some tests

As I reduce the non-crypto code base, I'm seeing (with increased frequency) errors saying certs are in wrong order.
I suspect there's something writing to temp files in the target dir and thus creating a race condition.
For now, I'm running tests with --test-threads=1, but this is a stopgap measure.

Fixed by adding mutex around OpenSSL FFI calls.


Extension Development: HTML Canvas in Webview only shows once

VSCode Version: 1.52.0
OS Version: Windows 10

Steps to Reproduce:

Clone the demo repository I created (https://github.com/JonasSchatz/webviewBugDemo) and open it in VS Code
Run the extension (F5)
In the Extension Development Host, run the WebviewDebugDemo: Show Graph command (Ctrl + Shift + P). You should see a graph visualization:

Close the webview and run the same command again. The graph visualization will, unexpectedly, not be rendered to the Webview. However, if you open the Webview Developer Tools, you will still see the message force-graph loading 300 nodes 299 links printed into the console, even though no graph is visible.

I had to restart my whole computer every time I wanted to see the visualization.

About the Demo:
All of the data for the Webview can be found in the static folder of the repository.
It contains a graph.js file, which just contains the copy-pasted demo code from the force-graph Javascript library (https://github.com/vasturiano/force-graph, first item in the list of examples). It builds on D3's force-graph, but renders to a HTML canvas instead of an svg, which MIGHT be the problem. I'm not an export on the HTML canvas, though, which is why I could not debug it. I had to switch from D3 to another library that can handle bigger force graphs due to performance issues.
There is also a dataviz_local.html file which you can open in your browser. It uses the same javascript files, but with hardcoded locations, so that you can see what it should look like.

Does this issue occur when all extensions are disabled?: Yes

Without further evidence, this sounds like an issue with your extension or the libraries you are using.
Please try narrowing down what doesn't work. If this really turns out to be a VS Code problem, then provide a minimal example that demonstrates the problem

mjbvz was right, it was NOT a problem with VS Code.
For anyone stumbling upon this problem in the future: When using the force-graph library as a part of your VS Code extension, you need to build it, which will give you a force-graph.x.y.z.min.js file. However, you also need to provide d3 (e.g. as d3.v6.min.js). force-graph will not complain about the missing dependency, it just won't show any data.
Caching made this a very tricky thing to detect. When testing in my browser, I still had d3 cached, so it worked without a problem. I expected VS Code to create completely empty webviews, but apparently there was some spillover, and sometimes it had a cached instance of D3 flying around somewhere, but not after closing and re-opening the webview.

Setting up Account and finalization


🐛 BUG: Issue with doing direct upload of sveltekit project with wrangler2

What version of Wrangler are you using?
2.0.6
What operating system are you using?
Mac
Describe the Bug
I have a monorepo project with following dir structure. Under apps/web I have a sveltekit project
\
- apps
   - web
- libs
- ....

When I build my project using the pages dashboard with following settings, everything works well.
Build output directory: /apps/web/.svelte-kit/cloudflare
Root directory: /

However if I try to publish by project using newly launched direct upload feature by executing following command
apps/web ➜ wrangler pages publish .svelte-kit/cloudflare/ --project-name <name>

While wrangler successfully uploads my project, on the published url, images are not loading - they have been kept under .svelte-kit/cloudflare/images folder. The generated layout.css file is getting downloaded with blank response. Though I can see valid file under .svelte-kit/cloudflare/_app/assets/pages/__layout.svelte-94e38b0d.css.
Am I missing something here?

Hey there! Thanks for reporting this, we're looking into a fix. In the meantime, you can try to cd to the directory before uploading:
cd .svelte-kit/ && wrangler pages publish ./cloudflare --project-name <name>

Thanks a ton @jhandsCF. This works for me and saves a bunch of time for me.

This should be fixed as of 2.0.15.


Compile under Mac

Hi,
I just compiled the code in Mac but when I ran it I got:
vtkExternalOpenGLRenderWindow (0x7f862e1e7200): VTK is designed to work with OpenGL version 3.2 but it appears it has been given a context that does not support 3.2. VTK will run in a compatibility mode designed to work with earlier versions of OpenGL but some features may not work.
Searching the web I found a possible solution which was to insert:
QSurfaceFormat::setDefaultFormat(QVTKOpenGLWidget::defaultFormat());
before main app is generated. This time the error message I got was:
QVTKFramebufferObjectItem::setVtkFboRenderer
QVTKFramebufferObjectRenderer::initScene()
QVTKFramebufferObjectRenderer::generatePlatform()
QVTKFramebufferObjectRenderer::updatePlatform()
Segmentation fault: 11
Any ideas?
Also, I was getting error when reading in an obj file (under Linux). Changing:
if (modelFilePathExtension == ".obj")
to
if (modelFilePathExtension == "obj")
fixed the problem....
Z

Just got it working on Mac, I upgraded Qt from 5.9.3 to 5.11.1. I also added the following line:
QSurfaceFormat::setDefaultFormat(QVTKOpenGLWidget::defaultFormat());
to CanvasHandler.cpp before QApplication app(argc, argv); is called.

@zaferl, While this does fix the problem in the instance, do you have any idea why this would be necessary in this instance?

If you look at the source code for where QVTKOpenGLWidget::defaultFormat() leads, it calls the defaultFormat function from QVTKOpenGLWindow, which returns the following custom surface format (which I copied and pasted to get my personal project to work). It would be nice if VTK output this from some other endpoint, so that it is easier to integrate into QtQuick.
QSurfaceFormat QVTKOpenGLWindow::defaultFormat()
{
  QSurfaceFormat fmt;
  fmt.setRenderableType(QSurfaceFormat::OpenGL);
  fmt.setVersion(3, 2);
  fmt.setProfile(QSurfaceFormat::CoreProfile);
  fmt.setSwapBehavior(QSurfaceFormat::DoubleBuffer);
  fmt.setRedBufferSize(8);
  fmt.setGreenBufferSize(8);
  fmt.setBlueBufferSize(8);
  fmt.setDepthBufferSize(24);
  fmt.setStencilBufferSize(8);
  fmt.setAlphaBufferSize(0);
  fmt.setStereo(false);
  fmt.setSamples(vtkOpenGLRenderWindow::GetGlobalMaximumNumberOfMultiSamples());
#ifdef DEBUG_QVTKOPENGL_WIDGET
  fmt.setOption(QSurfaceFormat::DebugContext);
#endif
  return fmt;
}

Source: https://gitlab.kitware.com/vtk/vtk/blob/master/GUISupport/Qt/QVTKOpenGLWindow.cxx

On the Mac with the changes of above the screen looks very badly rendered and interaction with the scene (selecting, rotating) is impossible. Any ideas what might cause this problem?

In Mac, I receive this error :
"ERROR: In .../VTK-8.1.1/Rendering/OpenGL2/vtkOpenGLRenderWindow.cxx, line 785
vtkGenericOpenGLRenderWindow (0x109966c00): GLEW could not be initialized.

brew install glew
kshahim<EMAIL_ADDRESS>schrieb am Sa., 25. Mai 2019, 14:34:

In Mac, I receive this error :
"ERROR: In .../VTK-8.1.1/Rendering/OpenGL2/vtkOpenGLRenderWindow.cxx, line
785
vtkGenericOpenGLRenderWindow (0x109966c00): GLEW could not be initialized.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nicanor-romero/QtVtk/issues/1?email_source=notifications&email_token=AABPRTDLOSYQUOW3UEJJ7QDPXEW6JA5CNFSM4FKINFYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWHRA2A#issuecomment-495915112,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABPRTGGCRNX2HH3X42INALPXEW6JANCNFSM4FKINFYA
.

Thanks Harmen, here is the message from brew :
Warning: glew 2.1.0 is already installed and up-to-date
To reinstall 2.1.0, run brew reinstall glew
but still i receive the same error

On 25 May 2019, at 14:37, Harmen Stoppels<EMAIL_ADDRESS>wrote:
brew install glew
kshahim<EMAIL_ADDRESS>schrieb am Sa., 25. Mai 2019, 14:34:

In Mac, I receive this error :
"ERROR: In .../VTK-8.1.1/Rendering/OpenGL2/vtkOpenGLRenderWindow.cxx, line
785
vtkGenericOpenGLRenderWindow (0x109966c00): GLEW could not be initialized.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nicanor-romero/QtVtk/issues/1?email_source=notifications&email_token=AABPRTDLOSYQUOW3UEJJ7QDPXEW6JA5CNFSM4FKINFYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWHRA2A#issuecomment-495915112,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABPRTGGCRNX2HH3X42INALPXEW6JANCNFSM4FKINFYA
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/nicanor-romero/QtVtk/issues/1?email_source=notifications&email_token=AGBIZISTPUZZNXZZRA7HAU3PXEXJFA5CNFSM4FKINFYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWHRCKA#issuecomment-495915304, or mute the thread https://github.com/notifications/unsubscribe-auth/AGBIZITW7NDQVGG6QVEFWALPXEXJFANCNFSM4FKINFYA.

the problem was related to VTK compilation, I enabled VTK_USE_SYSTEM_GLEW and now I received this error :
VTK is designed to work with OpenGL version 3.2 but it appears it has been given a context that does not support 3.2. VTK will run in a compatibility mode designed to work with earlier versions of OpenGL but some features may not work.
then, I added
QSurfaceFormat::setDefaultFormat(QVTKOpenGLWidget::defaultFormat())
it workes but the rendering quality compare to windows version is poor

Ie the rendering quality in Mac, somehow related to this?
https://gitlab.kitware.com/vtk/vtk/issues/17154

I think I find the reason why in Mac, the rendering quality is less than in windows,
in the following line, the macSize was divided by two while in windows, it is not the case:
QOpenGLFramebufferObject *QVTKFramebufferObjectRenderer::createFramebufferObject(const QSize &size)
{
QSize macSize = QSize(size.width() / 2, size.height() / 2);
...


running "bleep gen-native-image" on windows complains it can't find "ni"

So this is a known thing with graalvm native-image and windows. They fixed it for the next version of graalvm native image here https://github.com/oracle/graal/pull/5881 .
Basically for now you need some build setup on windows, https://www.graalvm.org/22.1/docs/getting-started/windows/ .

or hmm, maybe it should have found ni, maybe that's an error on my side.
Try running through those commands (and update the readme in this repo if possible if you get it working) and see where it fails. see if a ni binary has been downloaded with graalvms gu command


Watchtower plugin

Adds a c-lightning plugin based on the commitment_revocation hook from https://github.com/ElementsProject/lightning/pull/3601.
The plugin has the following methods:

registertower <tower_id> : registers the user id (compressed public key) with a given tower.
getappointment <tower_id><locator>: queries an appointment to a given tower.
list_towers: lists all the registered towers.
get_tower_info <tower_id>: gets all the locally stored data about a given tower.
retry_tower <tower_id>: tries to send pending appointment to a (previously) unreachable tower.

The plugin also has an implicit add_appointment method triggered via the  commitment_revocation hook.
The current approach has free registration and sends the data to all registered towers. There is no management of the tower subscriptions at the moment, so if the plugin runs out of available slots for a given tower the user will need to request more (via registertower).
Data sent to towers that are unreachable / there's a subscription error are backed up in pending_appointments. A retry mechanism is fired by default with an exponential backoff strategy. If the retry fails, it can be triggered manually via retry_tower.
Appointments flagged as invalid by the tower are stored in invalid_appointments. This should never happen to honest clients.
If the tower is caught misbehaving, for instance sending an invalid signature, it will be flagged as misbehaving and the proof will be stored in misbehaving_proof.
All data is stored on disk in a LevelDB.
Pending

More tests may be added before closing
Test with multiple towers at the same time

Ping @cdecker @darosior some tests are still missing, but I think the plugin is on a good shape now.  Feel free to give it a try.
I can setup a live instance of the tower so it's easier, just let me know.


Examples

Short notes from a conversation I’m having:

current contents of examples directory moved (to scratch or something)
new examples in nicely organized hierarchy in examples directory
they are runnable from terminal
they have all imports and whatever they need
special comment syntax let’s you omit parts from rendered html (like doctest directives)
rendered examples are copy paste able in their full form
every example turns into a test in the main test suite. Comment syntax let’s you add assertions that don’t get rendered
interactive interpreter examples are handles fully via sphinx’s mechanics

waiting for more examples!
thx


frontend build cleanup

PR contains:

latest npm dependencies
eslint migration
new build order: frontend-tests, frontend, unit-test, jar, integration-tests

*.scss files aren't picked up anymore!
label:bug

*.scss files aren't picked up anymore!

bug: #47.

Codecov Report

Merging #46 into canary will decrease coverage by 0.21%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             canary      #46      +/-   ##
============================================
- Coverage     84.49%   84.27%   -0.22%
============================================
  Files           167       92      -75
  Lines          3883     2048    -1835
  Branches        338      238     -100
============================================
- Hits           3281     1726    -1555
+ Misses          564      322     -242
+ Partials         38        0      -38

Impacted Files
Coverage Δ
Complexity Δ

...webapp/app/shared/constants/authority.constants.ts
100.00% <100.00%> (ø)
0.00 <0.00> (ø)

...o/github/bbortt/event/planner/EventPlannerApp.java

...rtt/event/planner/web/rest/InvitationResource.java

...ub/bbortt/event/planner/web/rest/RoleResource.java

...bbortt/event/planner/web/rest/AccountResource.java

...bbortt/event/planner/web/rest/SectionResource.java

...github/bbortt/event/planner/domain/Invitation.java

...nt/planner/security/UserNotActivatedException.java

...bortt/event/planner/web/rest/LocationResource.java

...va/io/github/bbortt/event/planner/domain/User.java

... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc8309b...d02dcec. Read the comment docs.


Create 'Under Construction' Page

DevTask: Create 'Under Construction' Page
Objective:
Implement an 'Under Construction' page to inform users that certain features are not yet available and integrate this page with the Calculators page to enhance user communication and experience.
Description:
Main Task Details:
1.  Create 'Under Construction' Page:

Develop a new view named under_construction.html.erb.
The design should be elegant and match the overall design theme of the site.
Include an appropriate image to visually represent the 'under construction' status.
Implement responsive and visually appealing styles for the message and image.

Additional Task Details:
2. Integrate 'Under Construction' Page with Calculators Page:

Modify the index action in CalculatorsController to render the 'Under Construction' page only in the production environment.

Acceptance Criteria

The 'Under Construction' page is created with appropriate styling.
The page is rendered only in the production environment for the Calculators page index action.

Done
https://docs.google.com/spreadsheets/d/1sujH4lmu4MjIP51k3ce_dvXDhQUZCx49H9asfORmCJs/edit?gid=1343828361#gid=1343828361&range=A1:D10


OCM-8053 | test: Automated id:43252 validating inputs for creating spot machinepools

output:
$ CLUSTER_ID=2b99vnpc3u6ktad4ad28ana0m67o7j7f ginkgo -focus 43252 tests/e2e/
Running Suite: e2e tests suite - /home/jericho/work/repos/rosa/tests/e2e
========================================================================
Random Seed:<PHONE_NUMBER>

Will run 1 of 67 specs
SSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS

Ran 1 of 67 Specs in 4.411 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 66 Skipped
PASS

Ginkgo ran 1 suite in 5.371714118s
Test Suite Passed

/lgtm

/lgtm

/test ?

/test e2e-presubmits-pr-rosa-hcp

The case is not executed in the Presubmit job. Something is wrong with Prow CI. We say the image is built, but do not find the built log. Compare with the [expected one|https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_rosa/2068/pull-ci-openshift-rosa-master-e2e-presubmits-pr-rosa-hcp/1792534480230027264/artifacts/] and the [failed one|https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_rosa/2053/pull-ci-openshift-rosa-master-e2e-presubmits-pr-rosa-hcp/1793198978569867264/artifacts/], we could not see the build-logs.

/test e2e-presubmits-pr-rosa-hcp

This case is a NonHCP case. We need to run it on the rosa classic cluster.
/test e2e-presubmits-pr-rosa-sts

/test e2e-presubmits-pr-rosa-sts

/lgtm


Fix retries

I'm not seeing any retries.
I'm very new to rust, so probably missing how it's supposed to work, but this change made retries work for me.

Thanks for the PR! Makes sense.
Can you add a test to check that it works?
Ideally, a unit test in the same file and an integration test in https://github.com/lycheeverse/lychee/blob/master/lychee-bin/tests/cli.rs. The integration test can be modeled after the existing tests. For example, you can use a mock http server as in this test: https://github.com/lycheeverse/lychee/blob/10056dc5e939be40a697bf30fecb9a9fdfec2d95/lychee-bin/tests/cli.rs#L547-L557
The unit test would be about creating a new status object and checking that it returns the correct result when calling should_retry() on it. For some reason, we don't have unit tests for that yet (hence why it's probably broken), but it's a great opportunity to change that. 😉

unit test and integration test added!

Brilliant! Thanks for the quick turnaround and for the clean implementation.


Disk encryption still using old keys after key rotation?

I have concerns about the key rotation in case of Azure disks. If Azure continues to use  "old" encryption key, then what is the point of rotating the key? Shouldn't Azure stop using the previous key completely? What if the previous key is somehow compromised? Key rotation isn't serving any purpose here.
Also this document is not consistent with other docs Azure has published. These specifics are nowhere else mentioned.

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 6e97c60e-0e31-ee4d-445e-392a46e1e617
Version Independent ID: 540ef37d-b439-47fc-9100-715e189f54b6
Content: Creating and configuring a key vault for Azure Disk Encryption - Azure Virtual Machines
Content Source: articles/virtual-machines/linux/disk-encryption-key-vault.md
Service: virtual-machines
Sub-service: disks
GitHub Login: @msmbaldwin
Microsoft Alias: mbaldwin

@pp511
Thanks for the feedback! I have assigned the issue to content author to check and update the document as appropriate.

@msmbaldwin
Can you please check and add your comments on this doc update request as applicable.

Thanks for your dedication to our documentation. Unfortunately, at this time we have been unable to review your issue in a timely manner and we sincerely apologize for the delayed response. If you are still experiencing a problem, you might want to move this question to Microsoft Q&A: https://learn.microsoft.com/en-us/answers/tags/94/azure-virtual-machines. Because this issue does not relate to documentation, we are closing this issue. #please-close


Improvement of authorization.

Codecov Report
Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 78.50%. Comparing base (7ac044c) to head (45cef0f).

Files
Patch %
Lines

...ponents/ferroamp_operation_settings/helpers/api.py
0.00%
29 Missing :warning:

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   80.61%   78.50%   -2.11%
==========================================
  Files          13       13
  Lines        1042     1070      +28
==========================================
  Hits          840      840
- Misses        202      230      +28

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


ValueError: cannot create object arrays from iterator

The following code snippet raises a ValueError:
import numpy as np
from pyodesys.symbolic import SymbolicSys

def rhs(f, g, u, c, k):
    """
    Make the rhs for coupled, controlled oscillators.
    .. math::
        \dot{x}_i = f(x_i) + \sum_j c_{ij} g(x_i, x_j) + k_iu(x_1,\ldots,x_n)

    :param f: onsite dynamics
    :param g: coupling function
    :param u: control function
    :param c: coupling matrix
    :param k: control switch
    :return: rhs
    """
    def inner(t, y, p):
        coupling = (sum(cij*g(xi, xj) for cij, xj in zip(ci,y)) for ci, xi in zip(c,y))
        coupling = np.fromiter(coupling, y.dtype)
        return f(y) + coupling + k*u(y)
    return inner

n = 3
f = rhs(lambda x: -x, lambda x, y: (x-y)**2, lambda x: 0, np.eye(n), np.zeros(n))

odesys = SymbolicSys.from_callback(f, n)

It works just fine if you use a list expression and coupling = np.array(coupling) tough.

Thanks for reporting this, however I think this is a NumPy specific issue, consider e.g.:
In [1]: import sympy as sp

In [2]: import numpy as np

In [3]: symbs = sp.symbols('x:5')

In [4]: v = (_ for _ in symbs)

In [5]: np.fromiter(v, object)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-480452c182a8> in <module>()
----> 1 np.fromiter(v, object)

ValueError: cannot create object arrays from iterator

Seems so. However, this works:
In [1]: c = (x for x in range(5))
    ...: np.fromiter(c, float)
Out[1]: array([ 0.,  1.,  2.,  3.,  4.])


Upgrade Existing Font to it's latest version?

What would need to happen to upgrade an existing Font to a new version?
I'm looking at FontAwesome 4.3.
Follow the same steps as outlined in https://github.com/fontello/fontello/wiki/How-to-suggest-font-to-fontello#how-to-prepare-a-font-for-use-with-fontello?

Someone can make PR to this repo http://forum.rcdesign.ru/rules/ or wait until i do it. Or anyone can import FA svg font as custom

closed in flavour of #384


fix: Webpack hashes change for not changed chunks when new ones are added.

Currently all compilation chunks are changing when new chunk is added to bundle (possibly removed too). This can break long term caching for often changing apps.
This change introducing consistent hashes via HashedModuleIdsPlugin (kinda described here).
Example:
Default generated app with following in index.js
import './style';

import { h } from 'preact';
import { Router } from 'preact-router';

import Header from './components/header';
import Home from './routes/home';
import Profile from './routes/profile';

// import NewRoute from './routes/new-route'

export default () => (
	<div id="app">
		<Header />
		<Router>
			<Home path="/" />
			<Profile path="/profile/" user="me" />
			<Profile path="/profile/:user" />
			{/* <NewRoute path="/new" /> */}
		</Router>
	</div>
);

After running npm run build build dir looks like this:
136B assets
 18K bundle.js
 15K favicon.ico
784B index.html
426B manifest.json
3.8K polyfills.chunk.906967bd865a58e79868.js
333B push-manifest.json
777B route-home.chunk.cb9a97a68a24844cb988.js
1.4K route-profile.chunk.912babe0ee7dbb610e0b.js
1.1K style.css
 86B style.css.map
3.9K sw.js

After adding new route:
136B assets
 18K bundle.js
 15K favicon.ico
784B index.html
426B manifest.json
3.8K polyfills.chunk.f9e3f954aa46b2631857.js
508B push-manifest.json
777B route-home.chunk.6a92827b1cd9460beb95.js
725B route-new-route.chunk.4f17c863583cb00f2870.js
1.4K route-profile.chunk.d01c8307b02769a19281.js
1.1K style.css
 86B style.css.map
3.9K sw.js

route-home.chunk had hash cb9a97a68a24844cb988 now it has 6a92827b1cd9460beb95. Same goes for other chunks.
After the changes hashes are correctly preserved.
Before:
136B assets
 18K bundle.js
 15K favicon.ico
784B index.html
426B manifest.json
3.8K polyfills.chunk.5156d40ad82df36bfb93.js
333B push-manifest.json
790B route-home.chunk.fe6f6d1271eb23067f8f.js
1.4K route-profile.chunk.94c8dcc7d504a8b9950f.js
1.1K style.css
 86B style.css.map
3.9K sw.js

After:
136B assets
 18K bundle.js
 15K favicon.ico
784B index.html
426B manifest.json
3.8K polyfills.chunk.5156d40ad82df36bfb93.js
508B push-manifest.json
790B route-home.chunk.fe6f6d1271eb23067f8f.js
732B route-new-route.chunk.ec70894bbedfe0afb3d3.js
1.4K route-profile.chunk.94c8dcc7d504a8b9950f.js
1.1K style.css
 86B style.css.map
3.9K sw.js

Chunks that were not changed have same hashes :sparkles:

This is beautiful.
Think it would be worth using shorter hashes for chunk filenames?

Idk, what's the benefit tough?
HtmlWebpackPlugin already deals with injecting them so that doesn't matter for end user IMHO.
The shorter the chunkhash the more possible hash collision becomes (probably doesn't matter too, unless we'd go for extremely short sizes 😜 ).

I just figured it'd be nicer to look at haha. It does slightly trim down the HTML payload size, though I agree it's pretty minimal.  Only raised the question because I've most often seen webpack hashes used truncated to 5 characters.

@developit I've looked into it and the docs don't mention base64 or smaller chunks anywhere. I've also tried using stuff like [name].chunk.[chunkhash:base64].js or [name].chunk.[chunkhash:5].js and as a result I've got files like: route-home.chunk.[chunkhash:5].js (i.e. no hash at all). Also base64 encoded number is longer than original and contains
I guess I could write a custom plugin which would trim down the length, don't know if it's worth the trouble. Your call though.

Hmmm... I'll check for a recent project of mine that uses [chunkhash:5] --- I'm pretty certain that still works!

I will have to update you in the morning, though. Currently in bed & past my bedtime =)

Definitely not worth a custom plugin, just figured if there was a builtin we could use it.

@rkostrzewski @developit
I went thru a few trials & the only combinations that work are [chunkhash:5] and [hash:5]. It looks like the base64 addition is loader-specific? I have it in some places, but it definitely doesn't work on output config names.

@lukeed you are right! Must have messed up something previously.
Anyway, I've set chunkhashes to 5 characters 🎉


the following pipeline is valid, but skl_to_pmml fails with a traceback error:

TypeError                                 Traceback (most recent call last)
/var/folders/3p/nkzqwh_177jgrfhcl035m1l80000gp/T/ipykernel_57171/2423173579.py in
7
8
----> 9 xgboost_to_pmml(pipeline, features, target_name, "/Users/huangshouxi/IdeaProjects/grocery/groceryqa-tools-predict/groceryqa-tools-predict-service/src/main/resources/model/xgb-pick.pmml")
~/opt/anaconda3/lib/python3.9/site-packages/nyoka/xgboost/xgboost_to_pmml.py in xgboost_to_pmml(pipeline, col_names, target_name, pmml_f_name, model_name, description)
74             categoric_values = pml_pp['categorical_feat_values']
75             mining_imp_val = pml_pp['mining_imp_values']
---> 76         PMML_kwargs = get_PMML_kwargs(model,
77                                       derived_col_names,
78                                       col_names,
~/opt/anaconda3/lib/python3.9/site-packages/nyoka/xgboost/xgboost_to_pmml.py in get_PMML_kwargs(model, derived_col_names, col_names, target_name, mining_imp_val, categoric_values, model_name)
116         Get the PMML model argument based on XGBoost model object
117     """
--> 118     algo_kwargs = {'MiningModel': get_ensemble_models(model,
119                                                       derived_col_names,
120                                                       col_names,
~/opt/anaconda3/lib/python3.9/site-packages/nyoka/xgboost/xgboost_to_pmml.py in get_ensemble_models(model, derived_col_names, col_names, target_name, mining_imp_val, categoric_values, model_name)
153     model_kwargs = sklToPmml.get_model_kwargs(model, col_names, target_name, mining_imp_val, categoric_values)
154     if 'XGBRegressor' in str(model.class):
--> 155         model_kwargs['Targets'] = sklToPmml.get_targets(model, target_name)
156     mining_models = list()
157     mining_models.append(pml.MiningModel(
~/opt/anaconda3/lib/python3.9/site-packages/nyoka/skl/skl_to_pmml.py in get_targets(model, target_name)
1094                 pml.Target(
1095                     field=target_name,
-> 1096                     rescaleConstant="{:.16f}".format(model.base_score)
1097 		)
1098             ]
TypeError: unsupported format string passed to NoneType.format

@vinayvinkumar 3_xgboost_With_PreProcess .ipynb made the same mistake

Hi @hsx1612727380, which version of xgboost are you using?

'1.5.0'

Hi @hsx1612727380, which version of xgboost are you using?

'1.5.0'

Currently, our exporter does not support xgboost with version >= 1.0. We are working on it and soon we will provide a release with that support.

The list of supported libraries and versions are mentioned here - https://github.com/SoftwareAG/nyoka#list-of-libraries-and-models-supported-by-nyoka-

The list of supported libraries and versions are mentioned here - https://github.com/SoftwareAG/nyoka#list-of-libraries-and-models-supported-by-nyoka-

Thanks

Hi @hsx1612727380, nyoka release 5.2.0 adds support for xgboost 1.x.x.


add regex expression for APM <> MSSQL relationship

Relevant information
The entity relationship between APM and MSSQL is currently only expecting to match on the hostname but sometimes this matching rule does not work.
Example that currently works fine is this:
datastore/instance/MSSQL/hostname/default
What is currently not matching is an example like this:
datastore/instance/MSSQL/abc123.mydomain.local/1663
The matching criteria abc123.mydomain.local from the APM metric does not match the Infra MSSQL entity's hostname of abc123.
This fix attempts to correct that by adding a regex expression that is looking to capture everything after datastore/instance/mssql/ and before the first . or / character.
Checklist

[x] I've read the guidelines and understand the acceptance criteria.
[x] The value of the attribute marked as identifier will be unique and valid.
[x] I've confirmed that my entity type wasn't already defined. If it is I'm providing an explanation above.

We, at coreint, have developed this relationship rule to work in specific scenarios. Our approach was a best effort; however, given the complexity and diversity of cases, it has been challenging to ensure coverage for all possible situations. The rule is designed to be effective when both the hostname and instance name are the same. In cases where they differ, the rule won't be applicable.
From the CoreInt perspective, if the rule not only covers its intended scenarios but also extends to additional use cases, that would be excellent.
Important: Who will be responsible for testing to ensure that these changes do not affect the current functionality? Without tests, it's difficult to be 100% sure.
Note: It's important to remember that there might be situations where APM uses the same hostname as the integration, which should theoretically match. However, the integration relies on the instance ID retrieved directly from the database.
cc'ing @mangulonr for awareness.

Important: Who will be responsible for testing to ensure that these changes do not affect the current functionality? Without tests, it's difficult to be 100% sure.

Definitely love to test data but I don't see any tests for any entity relationships at all; only entity definitions. Can you tell me (or send me a link to read up on) how tests were originally done (if they were done)?
Btw, looking for test data, I have discovered a third APM MSSQL datastore metric format that doesn't follow either use case for the PR which will lead to a disjointed Service Map experience:
Datastore/instance/MSSQL/{ip-address}/default
With the regex solution I've proposed, it would not break that experience any more than it already is, but I also don't know how to update the proposed PR with regex that will fix this additional use case.

I don't think any testing is available for relationships... 🤔  I assume maybe someone from @newrelic/entity-platform could help here.

I'm going to close this PR as it doesn't seem possible to modify this to work without creating false positives.


docs: update xeofs citation

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (main@787c598). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #145   +/-   ##
=======================================
  Coverage        ?   94.39%
=======================================
  Files           ?       73
  Lines           ?     6152
  Branches        ?        0
=======================================
  Hits            ?     5807
  Misses          ?      345
  Partials        ?        0

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


update deps

I'd like to clear out the dependabot pull requests.
Rust deps PRs
Closes #722 #719 #718 #717 #708 #707 #705 #622
Github Actions PRs
Closes #492 #547 #566 #658

@andygrove This PR just clears out the clean dependency bumps, and it's ready to go.


EDGINREACH-26 Refactoring oAuth2 login flow and tenant resolving

Purpose
While the record contribution and circulation endpoints for D2IR require x-from-code, x-to-code, and x-request-creation-time headers, the 3rd-party authentication endpoint should NOT require this header.
https://issues.folio.org/browse/EDGINREACH-26
Approach

Exclude headers from OpenAPI definition of token endpoint
Switched to localServerKey for tenant mapping resolving
Updated JWT token subject to localServerKey
Refactored exception handling to return 401 - invalid_token when tenant mapping is not found by local server key
Updated test JWT token to contain mandatory edgeApiKey claim
Update tests

TODOS and Open Questions

Learning

Pre-Merge Checklist:
Before merging this PR, please go through the following list and take appropriate actions.

Does this PR meet or exceed the expected quality standards?

[ ] Code coverage on new code is 80% or greater
[ ] Duplications on new code is 3% or less
[ ] There are no major code smells or security issues

Does this introduce breaking changes?

[ ] Were any API paths or methods changed, added or removed?
[ ] Were there any schema changes?
[ ] Did any of the interface versions change?
[ ] Were permissions changed, added, or removed?
[ ] Are there new interface dependencies?
[ ] There are no breaking changes in this PR.

If there are breaking changes, please STOP and consider the following:

What other modules will these changes impact?
Do JIRAs exist to update the impacted modules?

[ ] If not, please create them
[ ] Do they contain the appropriate level of detail?  Which endpoints/schemas changed, etc.
[ ] Do they have all they appropriate links to blocked/related issues?

Are the JIRAs under active development?

[ ] If not, contact the project's PO and make sure they're aware of the urgency.

Do PRs exist for these changes?

[ ] If so, have they been approved?

Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment.  Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.

README.md should be updated


Use dataflow analysis results to prune RHS guesses

Do not merge yet, posting to get reviews.
'Evaluate' needs more instructions implemented.

you should add some test cases that make sure pruning is working. I'm not sure how to best do this, perhaps by invoking a debug level that makes souper print how many guesses were pruned, and then check that.

Added a -souper-dataflow-pruning-stats-level option and modified test-pruning.sh to show number of guesses pruned.

pretty sure it looks good but I'd like to read your description of the strategy before merging-- how soon can you give me that?

Do you mean a short description that can be included as documentation in the code or the paper write-up?

pretty sure it looks good but I'd like to read your description of the strategy before merging-- how soon can you give me that?

Do you mean a short description that can be included as documentation in the code or the paper write-up?

Let's work towards the paper, maybe outline the overall strategy. Can be very informal, but do it in a latex template for your conference. I can make a private github repo for this if you want.

I think that in addition to end-to-end pruning test cases, when you add something complex like known bits analysis this needs some separate test cases

Added tests

LGTM, thanks!

please squash


refactor sass directory and add .gitignore file

Now using gulp-sass instead of the default file watcher from JetBrains IDE
css are placed within it's own file in assets directory
Added .gitignore file to exclude some of the hidden files in macOS

Thanks for the PR 😄

:ok_hand: :purple_heart:


Encoding / Decoding issue

The bracket characters (unsafe) and equal character (reserved) are directly set in the url query string or fragment without encoding.
example:
https://demo.algolia.com/instant-search-demo/?q=&hPP=10&idx=instant_search&p=0&nR[price][>=][0]=2676&is_v=1
should be:
https://demo.algolia.com/instant-search-demo/?q=&hPP=10&idx=instant_search&p=0&nR[price][>%3D][0]=2676&is_v=1
(same thing observed with useHash = true)

Hi @NicoD this should be handled by the browsers in a natural way: the url looks good , did you observe any bad behavior?

Hi @vvo ,
here some examples with a failure:

here in github, in my previous comment. When I copied the first url (which is the instantsearch url), github had itself encoded the unsafe characters (brackets) but not the =. The link does not work when following it. (however the second link - which i encoded manually - works fine)
i had the same issue when reading an instantsearch url through a slack client (not the web client)

=> i suppose this error may happened on others clients that encode the query string: the brackets are normaly url urlencoded, but not the = which is considered as a separator.

js url parsing lib (like urljs and browserify url) fail parsing the query string or fragment because of the = and the non encoded chars.

I think it would be safer to urlencode all the reserved and unsafe chars in the query string and the fragment (https://tools.ietf.org/html/rfc3986)

http://localhost:8080/?q=&hPP=24&idx=instant_search&p=0&nR[popularity][>=][0]=0&is_v=1

Test: http://<IP_ADDRESS>:8080/?q=&hPP=24&idx=instant_search&p=0&nR[popularity][>=][0]=0&is_v=1

@NicoD As for copy pasting the url from the urlbar or right click a link and copy from the context menu then paste it anywhere => it's the responsibility of the client to do things well. Github does, slack also it seems
On this issue, I was able to fix it by decoding the uri every time its needed (loading the page, back button): decodeURIComponent.
Seems to work well, let me know. The fix is incoming

This is not as easy as I thought, decoding when receiving has his owns quirks: values (facet refinements) with & or = are decoded and then mis interpreted as separators.

Test: http://<IP_ADDRESS>:8080/?q=&hPP=24&idx=instant_search&p=0&nR[price][>=][0]=2&nR[price][<=][0]=80&is_v=1


Panic in crypto/tls/common.go

What version of Go are you using (go version)?
go version go1.9 linux/arm64
Does this issue reproduce with the latest release?
We believe so but cannot currently confirm as it is very difficult to reproduce under controlled conditions (see below).
What operating system and processor architecture are you using (go env)?
linux, arm64
What did you do?
We are running a proxy which performs MITM interception on behalf of the user and typically is servicing hundreds of requests in parallel. We manage the certificate store by inserting certificates in the NameToCertificate map. Maps are not concurrency safe, so if a TLS handshake is occurring while a certificate is being inserted into NameToCertificate, a panic results. We avoid this in our code by protecting NameToCertificate with a mutex, but this is not possible in the native Golang code.
This error occurs randomly and we see it about once a week but have not been able to reproduce yet.
What did you expect to see?
We would like for our program to continue running, even if the handshake crashes. This could potentially be done by adding a recover() to the code in crypto/common.go around line 737:
    if cert, ok := c.NameToCertificate[name]; ok {
            return cert, nil
    }

What did you see instead?
The program crashes. This occurs even with the initial TLS handshake() protected by a recover. Stack trace:
fatal error: concurrent map read and map write
goroutine 11597 [running]:
runtime.throw(0xb64eb7, 0x21)
/usr/local/go/src/runtime/panic.go:605 +0x70 fp=0x44204919c0 sp=0x44204919a0 pc=0x42a600
runtime.mapaccess2_faststr(0xa6a400, 0x44202c0c00, 0x4429418a80, 0x18, 0x0, 0x4)
/usr/local/go/src/runtime/hashmap_fast.go:324 +0x488 fp=0x4420491a20 sp=0x44204919c0 pc=0x40d248
crypto/tls.(*Config).getCertificate(0x442023ec00, 0x4427736160, 0x2, 0x0, 0x0)
/usr/local/go/src/crypto/tls/common.go:737 +0xc4 fp=0x4420491aa0 sp=0x4420491a20 pc=0x5ab614
crypto/tls.(*serverHandshakeState).readClientHello(0x4420491c38, 0x4420491c28, 0x1138740, 0x577a84)
/usr/local/go/src/crypto/tls/handshake_server.go:216 +0x370 fp=0x4420491bf0 sp=0x4420491aa0 pc=0x5bd2f0
crypto/tls.(*Conn).serverHandshake(0x4428a0ce00, 0xb8ce98, 0x4428a0cf20)
/usr/local/go/src/crypto/tls/handshake_server.go:48 +0x68 fp=0x4420491d10 sp=0x4420491bf0 pc=0x5bcc88
crypto/tls.(*Conn).Handshake(0x4428a0ce00, 0x0, 0x0)

Are you building with a fork of the Go 1.9 source and a patch applied?
Also note the Config docs: https://golang.org/pkg/crypto/tls/#Config

A Config structure is used to configure a TLS client or server. After one
has been passed to a TLS function it must not be modified. A Config may be
reused; the tls package will also not modify it.

Looks like you'll have to explicitly synchronise the insertion of a cert with the config object itself, rather than just the NameToCertificateMap:
maybe the following would solve it?
replace Config type in your go code with a wrapper
type myConfig struct {
    sync.Mutex
    tls.Config
}

have the consumers of that Lock()/defer Unlock() and have the inserts in the NameToCertificateMap do the same.
I'm trying to say:  maybe you can just push the mutex one level out in Go?

Hi @WinstonPrivacy,
I think the error message is clear enough. There are some concurrent map operations going on. Have you tried running your application in -race mode ? That is a very easy way to figure out where the issue is.

We avoid this in our code by protecting NameToCertificate with a mutex, but this is not possible in the native Golang code.

These 2 statements are contradicting each other. If you already protect access with a mutex, then how is it not possible in the native Go code ?
Also, please post the code so that we can reproduce the issue on our side. However, I think this is a simple map access issue which you can identify with the -race mode.
Thanks.

Are you building with a fork of the Go 1.9 source and a patch applied?

I've succeeded in reproducing the problem in Go 1.9. I haven't tried other versions but I suspect it will be the same (need to confirm).

Also note the Config docs: https://golang.org/pkg/crypto/tls/#Config

A Config structure is used to configure a TLS client or server. After one
has been passed to a TLS function it must not be modified. A Config may be
reused; the tls package will also not modify it.

This is the problem right here. We're sending in a reference to the TLS Config. It looks like we should be copying it instead.

Looks like you'll have to explicitly synchronise the insertion of a cert with the config object itself, rather than just the NameToCertificateMap:
maybe the following would solve it?
replace Config type in your go code with a wrapper
type myConfig struct {
    sync.Mutex
    tls.Config
}

have the consumers of that Lock()/defer Unlock() and have the inserts in the NameToCertificateMap do the same but use a new tls.Config.
var myConfig myConfig
myConfig.Config = *theNewConfig

I'm trying to say: maybe you can just push the mutex one level out in Go?

This won't work because the original Golang code in tls/common.go does not use a lock. We are already locking in our code to avoid the panic but it crashes inside tls/common.go while doing the TLS handshake.

Hi @WinstonPrivacy,
I think the error message is clear enough. There are some concurrent map operations going on. Have you tried running your application in -race mode ? That is a very easy way to figure out where the issue is.

We avoid this in our code by protecting NameToCertificate with a mutex, but this is not possible in the native Golang code.

These 2 statements are contradicting each other. If you already protect access with a mutex, then how is it not possible in the native Go code ?
Also, please post the code so that we can reproduce the issue on our side. However, I think this is a simple map access issue which you can identify with the -race mode.
Thanks.

I suppose we could modify the Golang code in crypto/tls/common.go to expose a public lock. This would be complicated and terribly bad design, but it's not practical in any case because we don't want to maintain our own fork of Golang.

I now have working code which consistently reproduces the problem. I've pasted it into Go Playground but because it uses external libraries, you'll have to run it locally.
https://play.golang.org/p/RlKeIbk8H9h

The bottom of your code contains a private key. I'll take a read through, but it sounds like you should be copying the Config and not modifying it.
var CA_KEY = []byte(`-----BEGIN RSA PRIVATE KEY-----

That's a throwaway key for unit testing only. It's never used for anything else.

I'm trying to say: maybe you can just push the mutex one level out in Go?

This won't work because the original Golang code in tls/common.go does not use a lock. We are already locking in our code to avoid the panic but it crashes inside tls/common.go while doing the TLS handshake.

From your example, as @adamdecaf noted, you're locks aren't locking the whole TestCaConfig object.  I think it would work if you modified it so that every time a domain is inserted, a whole new tls.Config is created or copied from the old one.
To be more explicit, suppose we add a global variable theTlsConfig to your example, and an associated mutex theTlsMutex.
line 97 would have something like
TLSClientConfig: getTlsConfig()

where getTlsConfig is a function you define which returns the current TlsConfig with respect to all such configs that have been created (one for each insertion).
Then lines 73-75 would have to be replaced by something like
 setTlsConfig(tlsc)

Then
func getTlsConfig() *tls.Config {
   theTlsMutex.Lock()
   defer theTlsMutex.Unlock()
   return &theTlsConfig
}

fun setTlsConfig(domain, tlsc) {  // sorry I didn't get the types for domain, tlsc but I think you get the point...
theTlsMutex.Lock()
defertheTlsMutex.Unlock()
tmp := theTlsConfig
tmp.NameToCertificateMap[domain] = tlsc
theTlsConfig = tmp
}

It is awkward, but I would guess that would work for the example

disclaimer: I didn't run it or test a variation, just seems like it would work.

If you need to crypto/tls to use your mutexes, the answer is to define the GetCertificate func:
https://golang.org/pkg/crypto/tls/#Config.GetCertificate
It is used before NameToCertificate and you can do any locking in there.

Thanks. I've modified the code to manage a pool of tlsconfigs, rather than having just one with potentially thousands of hosts in it. This is much cleaner.

Thanks. I've modified the code to manage a pool of immutable tlsconfigs, rather than having just one mutable tlsconfig with potentially thousands of hosts contained in its map. This is much cleaner.

Yes, my awkward fix is a memory hog.  @bradfitz's solution is the correct one and doesn't have this problem, just couple a mutex with a map and use the GetCertificate attribute to coordinate.

@bradfitz's solution is a good workaround but not ideal from a performance standpoint because you're introducing locks in critical code. Keeping an immutable tls config around is far better because you avoid lock contention.

@bradfitz's solution is a good workaround but not ideal from a performance standpoint because you're introducing locks in critical code. Keeping an immutable tls config around is far better because you avoid lock contention.

Glad to hear it works.

Thanks everyone for your help. We were able to reconfigure things so that instead of using a single TLSConfig, we're now using a cache of immutable TLSConfigs for each website. This has cured the panics!

Good to hear!  But with my suggestion, I hope you are making a full copy of the map, instead of what I suggested where in setTlsConfig the line tmp := theTlsConfig only copies a reference to the NameToCertificateMap

We ended up creating a map of TLSConfigs and we pass out references to those (they do not change once created). Each of those has their own NameToCertificate map.
I'm curious though why you suggest copying the TLSConfigs instead of passing a reference?


Add support for #{$var} syntax (postcss-scss)

You can already use a $i variable by wrapping it as $(i). Using { and } would be very confusing, as those are typically nesting characters.

#{$var} syntax is valid at scss
http://sass-lang.com/documentation/file.SASS_REFERENCE#interpolation_
and will be useful when used in conjunction with postcss-scss parser
https://github.com/postcss/postcss-scss

I updated this to work with the latest testing code from upstream at https://github.com/adam-h/postcss-advanced-variables/commit/bee2d3c00bcd56268f65cd607603218d3e94f4f9


Emissions Module Oracle to Postgres Schema Migration Master Data Tables

Follow the data migration process using the AWS Schema Conversion tool to convert the following tables from Oracle to Postgres and apply to the cloud.gov dev database.
Please follow the schema order, group order, & table load ranking order within each group as presented below in order to prevent constraint errors applying to the dev database. If you encounter any errors while converting or applying to the database please STOP and immediately contact Jason Whitehead.
Reminder to check & verify datatypes for each column paying close attention to date & time columns...

userid column needs to change from character varying(8) to character varying(25)
Audit columns that contain date and time information such as add_date, update_date, last_updated, etc. need to be changed after conversion from timestamp with time zone to timestamp without time zone
Columns that just contain date only information (no time info) with a separate but related hour column such as being_date, begin_hour, end_date, & end_hour the datatype should stay as date in Postgres

Please check the box beside each table once they are converted, placed into dev, and ready to be loaded. The number in front of the table is the load level ranking for the Informatica spreadsheet!!!
CAMDECMPSMD Schema

[x] 1-FUEL_FLOW_PERIOD_CODE
[x] 1-HOUR_MEASURE_CODE
[x] 1-HOURLY_TYPE_CODE
[x] 1-NSPS4T_ELECTRICAL_LOAD_CODE
[x] 1-NSPS4T_EMISSION_STANDARD_CODE
[x] 1-OPERATING_TYPE_CODE
[x] 1-SAMPLE_TYPE_CODE
[x] 1-SOD_MASS_CODE
[x] 1-SOD_VOLUMETRIC_CODE
[x] 1-SORBENT_TRAP_APS_CODE
[x] 1-TRAIN_QA_STATUS_CODE

checked dev db and all looks good


Replace orientation with custom page size

Enhancement replacing limited U.S. letter size portrait/landscape orientation parameter with more flexible pageSize parameter, per #56 .

@KeithGillette  Is there a way to make this backward compatible? Otherwise, this becomes a breaking change and will need a major release.

@KeithGillette I feel it would be best if you could rebase the changes with develop, because the target branch is develop and right now after changing it, its lists down a lot of commits which are specific to master only.