I believe this section of the docs is incorrect and could be improved

https://github.com/elixir-ecto/ecto_sql/blob/427468fb653dde0d0150aaa8d2bb2feeca31838d/lib/ecto/adapters/sql/sandbox.ex#L249
I think this section of the docs is incorrect or at least misleading and could be improved.

For example, imagine multiple tests are
trying to insert the same user to the database. They will attempt to
retrieve the same database lock, causing only one test to succeed and
run while all other tests wait for the lock.

How can this ever happen? Each test uses its own transaction, isolated from the rest. Even if different tests try to insert the same data, they will never cause a conflict because the transactions of the sandbox are never committed. In fact, my tests do this all the time: each test inserts its own data calling the same fixture functions. The primary keys of the rows they insert overlap a lot, and I've never seen a conflict. There can't be conflicts if you're not committing a transaction.
Also which database lock will they "attempt to retrieve" when inserting a row? No locks are acquired when inserting a new row.

For example, instead of:
def insert_user do
Repo.insert! %User{email<EMAIL_ADDRESS>end
prefer:
def insert_user do
Repo.insert! %User{email<EMAIL_ADDRESS>end
defp counter do
System.unique_integer [:positive]
end

I think this is unnecessary for the reasons stated above: each test operates in its own transaction, which is a completely isolated world. Different tests can insert the same row and they will never see each other's data.
I think that the concerns stated here apply to the situation where the test data is inserted before running the tests (i.e. not within the sandbox transaction), for example with a test DB that has been seeded beforehand. In that case, yes, different tests may try to acquire a lock on the same rows and cause deadlocks. However, this should never be a problem if the test data is inserted from within the sandbox transaction.
Do you agree? Or am I missing something important?
Thanks!

It is missing the information there is a unique index on the email column. I will update.

Hope this helps to understand how it happens: https://gist.github.com/v0idpwn/dc621ca348837a590dac01fd909126e2

FWIW, the deadlock in the insert case occurs because each transaction is waiting on the other to commit before it can proceed.
How this is implemented in postgres is that a transaction takes an "exclusive" lock on its own transaction id.  When another transaction comes along and sees the potential for a conflict (i.e. with inserting the same unique value) it will attempt to take a "share" lock out on the other transaction's id. But this has to wait for the original transaction's "exclusive" lock to disappear (i.e. for that transaction to commit/rollback). So when both transactions are waiting on each other in this way, there is a deadlock.
Reference:
https://www.postgresql.org/docs/current/view-pg-locks.html

Throughout running a transaction, a server process holds an exclusive lock on the transaction's virtual transaction ID. If a permanent ID is assigned to the transaction (which normally happens only if the transaction changes the state of the database), it also holds an exclusive lock on the transaction's permanent transaction ID until it ends. When a process finds it necessary to wait specifically for another transaction to end, it does so by attempting to acquire share lock on the other transaction's ID (either virtual or permanent ID depending on the situation). That will succeed only when the other transaction terminates and releases its locks.

Thanks @greg-rychlewski that's the bit of information I was missing!
I have to confess I had no clue postgres was that smart and would try to detect whether 2 transactions can potentially create a deadlock when inserting new data. I'm a bit baffled as to why my async tests never see this problem (I've never seen a deadlock!) even though they always insert the same data. What I suspect it's happening is that one of my test is waiting on the other to terminate and release the lock. That would mean that the speed of my test suite could be improved. I'll test this out.
Again, I had no idea DB would perform that kind of check during insertion, I was assuming all checks would be performed when trying to commit the transaction. Apparently I learned something new today.

Also just in general, transactions are not completely in their own world when it comes to writing. You cannot update the same row as another transaction until it has committed. If this wasn't the case then imagine a scenario where 2 transactions are incrementing the same row:

My point was specifically about writing new data, not updating existing data. When it comes to updating existing data, I was well aware that different transactions could (and should) be able to get on each other's toes.

@v0idpwn thanks for your example, too!


Added a table of contents to the top of README

Description
Added a table of contents to the top of the README.md. Because the README is quite long and dense, this will help future cohorts skim the doc and get an idea of its contents quickly, and hopefully streamline the onboarding process.
Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[X] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update

Change Status

[X] Complete, tested, ready to review and merge
[ ] Complete, but not tested (may need new tests)
[ ] Incomplete/work-in-progress, PR is for discussion/feedback

Has This Been Tested

[ ] Yes
[ ] No
[X] Not necessary

Checklist

[X] My code follows the style guidelines of this project
[X] I have performed a self-review of my own code
[X] I have commented my code, particularly in hard-to-understand areas
[X] I have made corresponding changes to the documentation
[X] My changes generate no new warnings
[X] I have added tests that prove my fix is effective or that my feature works
[X] New and existing unit tests pass locally with my changes
[X] There are no merge conflicts

Great job on adding this helpful item @TemsyChen!


Support for Java 8 Streams

Add assertions for streams, it should contain at least the same assertions as for a List.

Something feels the same way wrong here as by iterator assertion. A stream is not a data structure and especially no list. Having said that i can imagine that there are usecases where you have to check the results of a stream chain (e.g. mapping, filtering...).
So basically we have to add a convenience method have we?

Ok, for start, Stream assertions should have same API methods as Iterable assertions?

I guess it will become more and more common to have methods return streams so I definitely see a use case for stream assertions. :+1:
For others coming here looking for a solution: Depending on the tested method's semantics regarding order and uniqueness of stream elements, I currently usually collect them into a list or set and assert the desired properties there.

Implemented by converting the Stream to a List and then returning a ListAssert.

The error report does not show the contents of the expected stream:
assertThat(Stream.of(1)).isEqualTo(Stream.of(2));
org.junit.ComparisonFailure:
Expected :java.util.stream.ReferencePipeline$Head@14f232c4
Actual   :[1]

@dpinol it does not indeed, actual is converted to a list  that's why is is displayed "nicely".
We could display expected like a list for isEqualTo but it might be misleading because if will never be equal to actual because actual is a list but even if it was a Stream it would not be equal, you can verify that by executing Stream.of(1, 2, 3).equals(Stream.of(1, 2, 3)) which returns false.
I'm not sure I want
assertThat(Stream.of(1, 2, 3)).isEqualTo(Stream.of(1, 2, 3));

to return an error like:
org.junit.ComparisonFailure:
Expected :Stream(1, 2, 3)
Actual   :Stream(1, 2, 3)

that would hide the fact that Stream don't override equals.
I think here AssertJ does not a great job at showing its limitations, Stream assertions have been implemented by converting actual to a list and then using list assertions, that works well for most of the assertions except for assertions like isEqualTo or isSameAs.
It may be time to have first class support for Stream, if we do so there will always be other limitations, no assertions chaining as you can only consume a stream once.

I don't think there is an ideal way of implementing Stream, converting to list assertions is nice as it supports assertions chaining. One thing we should do is to clearly inform users of the limitations/pitfalls of Stream assertions (the same applies for Iterator assertions btw)
@PascalSchumacher thoughts ?

I agree.

So can you please enlight us of the limitations/pitfalls of Stream assertions ? @joel-costigliola

@iheb719  will do ... in the doc. but to answer quickly the main thing is we are converting the  stream to a list under the hood so any assertions assuming it is dealing with a Stream would not work, eg:
assertThat(stream).isInstanceOf(Stream.class);


Default constructor issue

According to the demo, I deserialize my class object (for example MyClass) using mergeDelimitedFrom function. However, for the historical reason, MyClass has some initialization operation like list.add("hello") and I could not modify this. So before I deserialize my instance, it has been initialized and this action will do twice after I invoke mergeDelimitedFrom. Is there a way to figure out?
Here is my code:
MyClass mc = new MyClass();
FileInputStream input = new FileInputStream(new File("text"));
Schema<MyClass> schema = RuntimeSchema.getSchema(MyClass.class);
LinkedBuffer buffer = LinkedBuffer.allocate(LinkedBuffer.DEFAULT_BUFFER_SIZE);
ProtostuffIOUtil.mergeDelimitedFrom(input, mc, schema, buffer);

PS: Also, I have tried another function parseListFrom but still not work.

Hi, can you please provide full code that reproduces a problem?

Here is my testing code~
public class test {
    public static void main(String[] args) {
        try {
            Schema<MyClass> schema = RuntimeSchema.getSchema(MyClass.class);
            LinkedBuffer buffer = LinkedBuffer.allocate(LinkedBuffer.DEFAULT_BUFFER_SIZE);

            FileOutputStream output = new FileOutputStream(new File("test.txt"));
            MyClass myClass = schema.newMessage();
            ProtostuffIOUtil.writeDelimitedTo(output, myClass, schema, buffer);
            output.close();

            System.out.println(myClass.getList().size()); // 1

            FileInputStream input = new FileInputStream(new File("test.txt"));
            MyClass myClassNew = schema.newMessage();
            buffer.clear();
            ProtostuffIOUtil.mergeDelimitedFrom(input, myClassNew, schema, buffer);
            input.close();

            System.out.println(myClassNew.getList().size()); // 2

        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

class MyClass {
    private List<String> list;
    public MyClass() {
        list = new ArrayList<>();
        list.add("hello");
    }
    public List<String> getList() {
        return list;
    }
}

Ok, I faced with similar issue in the past. In general, this is expected behavior: if class has a default constructor, then it is used for creating new object instances.
In your case two things happen:

Default constructor adds an element to list when you create a message instance.
When you call ProtostuffIOUtil.merge..., it deserialize data and add other element to list.

As a possible solution that might work for such cases, we can force RuntimeSchema to use sun.reflect.ReflectionFactory#newConstructorForSerialization. But this solution is really unsafe and not portable (it will not work on Android).
I will check what we can do on next week.

Thx for your reply :). I will have a try. And there is a doubt in my mind why not design some not-merge functions for deserializing and just accept an object. Hope to hear from you soon.

Hi,
I found a solution.
There is a system property that forces protostuff runtime to always use constructor for serialization: protostuff.runtime.always_use_sun_reflection_factory.
You should set this property to true on startup:
System.setProperty("protostuff.runtime.always_use_sun_reflection_factory", "true");

Here is my unit test code:
package io.protostuff.runtime;

import java.io.ByteArrayOutputStream;
import java.util.ArrayList;
import java.util.List;

import org.junit.Assert;
import org.junit.Test;

import io.protostuff.LinkedBuffer;
import io.protostuff.ProtostuffIOUtil;
import io.protostuff.Schema;

/**
 * @author Kostiantyn Shchepanovskyi
 */
public class AlwaysUseSunReflectionFactoryOptionTest {

	@Test
	public void forceUseSunReflectionFactory() throws Exception {
		System.setProperty("protostuff.runtime.always_use_sun_reflection_factory", "true");
		Schema<MyClass> schema = RuntimeSchema.getSchema(MyClass.class);
		ByteArrayOutputStream output = new ByteArrayOutputStream();
		MyClass myClass = new MyClass(); // constructor initializes list with one element
		ProtostuffIOUtil.writeTo(output, myClass, schema, LinkedBuffer.allocate());
		byte[] bytes = output.toByteArray();
		Assert.assertEquals(1, myClass.getList().size());
		MyClass myClassNew = schema.newMessage(); // default constructor should not be used
		ProtostuffIOUtil.mergeFrom(bytes, myClassNew, schema);
		Assert.assertEquals(1, myClassNew.getList().size());
	}

	final static class MyClass {
		private List<String> list;

		public MyClass() {
			list = new ArrayList<>();
			list.add("hello");
		}

		public List<String> getList() {
			return list;
		}
	}

}

Hi, @kshchepanovskyi it's a really nice solution and help me a lot. Thx~


experiment: use e2-standard-2 for postdeployment

This experiment is to confirm the test suite has no issues switching to an e2 GCE instance type in advance of considering changing from n1-standard-1 to a more available type such as the e2 series.

Passing tests on this PR establish that updating the deployment process to a new machine type should be trivial.
Next step: confirm the recommended machine type.

(e2 is not in fact more available)


refactor: Improve ProjectHeader layout responsiveness

What does this PR do and why?
This commit improves the ProjectHeader layout's responsiveness. Changes include reorganizing the layout for various screen sizes, updating the position of SocialLinks, and ensuring that the ProjectSubheading and ProjectDescription are correctly aligned on different devices. These improvements enhance the user experience by providing a consistent and visually appealing interface across various screen sizes.
Fixes JB-196
Screenshots or screen recordings
If applicable, provide screenshots or screen recordings to demonstrate the changes.
Acceptance checklist

[ ] I have evaluated the Approval Guidelines for this PR.
[ ] (if relevant) I have tested this PR in all supported browsers.
[ ] (if relevant) I have tested this PR in dark mode and light mode (if applicable).

Current dependencies on/for this PR:

main

PR #3330   👈

This comment was auto-generated by Graphite.


Issue with editor

Greetings, I'm having some errors with the editor. The console outputs some messages:
ReferenceError: SPE is not defined
CONFIG.editor.js:222:3
TypeError: CONFIG.editor is undefined
CONFIG.settings-panel.js:24:13
THREE.WebGLRenderer 71 THREE-r71.min.js:523:216
ReferenceError: SPE is not defined

Hi @SakyaStelios, I'm afraid the Editor only supported older versions of the engine. It's on my (very long list) of things to fix :)


Feature request: Update badge to show build status of main branch

Feature description
You should update the build status badge to this link:
https://github.com/requests-cache/aiohttp-client-cache/actions/workflows/build.yml/badge.svg?branch=main
We want to see the status of the main branch, also GitHub caches things quite aggressively, so it might stay red for a while if a build failed.

Done. Thanks for the tip!


fix for when keyframes are combined.

When I have created multiple keyframes (dynamically via sass) they are combined without semi-colon.
Apparently, the document parser trims trailing semi-colons from stylesheet rules..

Ping @kodybrown, please please post us an example so we can get this pull request wrapped up :gift:

@Prinzhorn I had a look at the tests, looks ok to me. Lets get this merged.

@evolve2k I've added you as a collaborator. Feel free to give some new live to this repo.


libopus: 1.1.5 -> 1.2

Motivation for this change
Beside the new version I also changed the download URL because on the official homepage they link to archive.mozilla.org while downloads.xiph.org seems to be only a mirror which is currently not up2date.

http://opus-codec.org/release/stable/2017/06/20/libopus-1_2.html

Changes since 1.1.x include:

Speech quality improvements especially in the 12-20 kbit/s range
Improved VBR encoding for hybrid mode
More aggressive use of wider speech bandwidth, including fullband speech starting at 14 kbit/s
Music quality improvements in the 32-48 kb/s range
Generic and SSE CELT optimizations
Support for directly encoding packets up to 120 ms
DTX support for CELT mode
SILK CBR improvements
Support for all of the fixes in draft-ietf-codec-opus-update-06 (the mono downmix and the folding fixes need --enable-update-draft)
Many bug fixes, including integer wrap-arounds discovered through fuzzing (no security implications)

Things done

[X] Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)

[X] NixOS
[ ] macOS
[ ] Linux

[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
[X] Tested execution of all binary files (usually in ./result/bin/)
[X] Fits CONTRIBUTING.md.

Thanks!


Prerequisites of Local Environment are missing in README

Describe the issue:
In the local environment , it need to install and open Docker, but the Prerequisites in README are not specified.

Suggestion:
Add Docker into the Prerequisites in README.
@hemarina and @manekinekko for notification.

Fixed by #11


Fix asset root on Heroku

The asset root needs to be set, otherwise the assets can't be served.

@fofr I'm seeing errors on the review app with:
Failed to find a valid digest in the 'integrity' attribute for resource 'https://govuk-frontend-app-pr-1461.herokuapp.com/frontend/application-9468d9f94e70184efa7a2553abab993e5a1b8a750de61c49c36c7cd22aaa1046.css' with computed SHA-256 integrity '47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU='. The resource has been blocked.

Any idea?


Test auth

Description
configured client side testing. drafted initial test suites for login and register components with minimum of 60% code coverage.
How to test
navigate to client folder. run npm i. run npm test. test script should run and display results and code coverage report. 100% of tests should pass and report should show about 66% code cover overall and for Login and Register components.
Notes
Jest must be v26, not 27.
Issue tracking
https://trello.com/c/Qo776MSx/40-authentication-test-suites

 Authentication test suites


keys: support returning legacy verification script

Closes #1110 .
When one works with legacy (NEO2) wallets, it is useful to have access
to old verification script formats.

OK, I'll accept it as a temporary preview2 fix and will fix it later.


Hide deprecated CLI flags

Pull Request Motivation
These flags were deprecated as part of the 1.13 release.
Now, I would like to hide them so the --help output is a lot cleaner.
Kind
/kind cleanup
Release Note
NONE

/approve


[Actions] auto approve before auto merge

What type of PR is this：

[ ] bug
[ ] feature
[ ] enhancement
[ ] refactor
[ ] others

Which issues of this PR fixes ：

Fixes #
Problem Summary(Required) ：

Checklist:

[ ] I have added test cases for my bug fix or my new feature
[ ] I have added user document for my new feature or new function

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.dulong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.You have signed the CLA already but the status is still pending? Let us recheck it.


fix(theories/topology.md) update set complement and set subtraction notation

Fixes an example that @jcreedcmu noticed to be broken.
Set complement notation changed in https://github.com/leanprover-community/lean/pull/367 and https://github.com/leanprover-community/mathlib/pull/3212.
I suspect something similar happened with set subtraction, but I have been unable to find
the exact point in history.

Thanks!


Gutter for git-crypted files is borked

Description
git-crypt allows transparently encrypting files with git, where the checked-out file is decrypted, but the on in the repository itself is encrypted.
Files encrypted in such way always show that lines have been modified/added in the gutter.
Under the hood, this uses .gitattributes with filter=git-crypt diff=git-crypt, but I think this is not taken into account by gitsigns.
Neovim version
NVIM v0.7.0
Operating system and version
Linux 5.18.7-arch1-1 GNU/Linux
Expected behavior
Files should show correct gutter, or perhaps none if unsupported?
Actual behavior
Gutter shows wrong. Likely the result of not taking gitattributes/diff into account.
Minimal config
vim.o.packpath = '/tmp/nvim/site'

local plugins = {
  gitsigns = 'https://github.com/lewis6991/gitsigns.nvim',
  -- ADD OTHER PLUGINS _NECESSARY_ TO REPRODUCE THE ISSUE
}

for name, url in pairs(plugins) do
  local install_path = '/tmp/nvim/site/pack/test/start/'..name
  if vim.fn.isdirectory(install_path) == 0 then
    vim.fn.system { 'git', 'clone', '--depth=1', url, install_path }
  end
end

require('gitsigns').setup{
  debug_mode = true, -- You must add this to enable debug messages
  -- ADD GITSIGNS CONFIG THAT IS _NECESSARY_ FOR REPRODUCING THE ISSUE
}

-- ADD INIT.LUA SETTINGS THAT IS _NECESSARY_ FOR REPRODUCING THE ISSUE

Steps to reproduce

Just set up a repo with git-crypt: https://github.com/AGWA/git-crypt#using-git-crypt
Create a text file, make sure it's set to be encrypted.
Commit it
Open the file with neovim.

Gitsigns debug messages
(no output)

This looks like a duplicate of #480

Yeah, they seem to be quite related. Not entirely the same, but it's likely possible to fix both together.
Let's continue on #480.


lower minSdkVersion

Does this library rely on minSdk 16? Our project still uses minSdk 15.

Yes, because I don't have the interest, time or patience to support anything earlier.

For future reference, you can just ignore the library's minSdk by adding this to your manifest:
<uses-sdk tools:overrideLibrary="com.readystatesoftware.chuck"/>

It's mostly safe, because this is just a debug feature and in case it fails when running on sdk 15... not a big deal.


Compatibility with PokemonGo-Map JSON

I'm trying to get this to work with the python client's web UI. The sample data looks like this:
I'm just poking around right now and trying to figure out where these values came from (and how they were converted from the binary formats and s2 formats, etc). If there's anything you can immediately comment on please do so. Otherwise I'll just be working my way through. :)
{
    "pokemons": [
        {
            "disappear_time":<PHONE_NUMBER>156,
            "encounter_id": "MTUzMjQxMjY4NTg4NDUyMTExMTc=",
            "latitude": 40.36915523640919,
            "longitude": -111.75098587678943,
            "pokemon_id": 16,
            "pokemon_name": "Pidgey",
            "spawnpoint_id": "874d84035d7"
        }
    ],
    "gyms": [
        {
            "enabled": true,
            "guard_pokemon_id": 59,
            "gym_id": "ffb14e36c7344c289457faa20319681c.16",
            "gym_points": 10495,
            "last_modified":<PHONE_NUMBER>398,
            "latitude": 40.360204,
            "longitude": -111.761231,
            "team_id": 2
        }
    ],
    "pokestops": [
        {
            "active_pokemon_id": 19,
            "enabled": true,
            "last_modified":<PHONE_NUMBER>759,
            "latitude": 40.370669,
            "longitude": -111.755525,
            "lure_expiration":<PHONE_NUMBER>716,
            "pokestop_id": "1ef6996b547746c69e22dbe73af6fbb0.16"
        },
        {
            "active_pokemon_id": null,
            "enabled": true,
            "last_modified":<PHONE_NUMBER>717,
            "latitude": 40.364965,
            "longitude": -111.756411,
            "lure_expiration": null,
            "pokestop_id": "1494e52456d64e979fdee61437bfefa7.16"
        }
    ]
}

Almost got pokemons figured out:
            "disappear_time": MapPokemon[i].ExpirationTimeMs // how to convert from ProtoLong to int?
            "encounter_id": MapPokemon[i].EncounterId // how to convert from ProtoLong to base64?
                                      // also NearbyPokemon[i].EncounterId
            "latitude": MapPokemon[i].Latitude
            "longitude": MapPokemon[i].Longitude
            "pokemon_id": MapPokemon[i].PokedexTypeId // how to convert from ProtoLong to base64?
                                      // also NearbyPokemon[i].PokedexNumber
            "pokemon_name": pokeio.pokemonlist[n - 1]
            "spawnpoint_id": MapPokemon[i].SpawnpointId

Both of ExpirationTimeMs and EncounterId should be protoLong. However, they can be fit into numbers. You can simply do parseInt(MapPokemon[i].ExpirationTimeMs.toString()) to transform it back into number.

The gym stuff comes from Fort in a heartbeat. This API is missing some of the data, but it looks something like this:
Fort: [
{
FortId: "076c7d8c03634557bf9a90ce51028268.16",
LastModifiedMs: {
low: 162407318,
high: 342,
unsigned: false
},
Latitude: 38.898951,
Longitude: -77.038961,
Team: null,
GuardPokemonId: null,
GuardPokemonLevel: null,
Enabled: true,
FortType: 1,
GymPoints: null,
IsInBattle: null,
ActiveFortModifier: null,
LureInfo: null,
CooldownCompleteMs: null,
Sponsor: null,
RenderingType: null

If you'd like to help I now have a working server (using this library) and client (forked from AHAAAAAAA) at https://github.com/Daplie/node-pokemap
Right now it only shows Pokémon, but with a little help massaging the data to match AHAAAAAAA's format, it'll show all the rest as well.

How did you get the pokemon locations?


Travis CI is possibly broken

Checklist

[ ] I'm reporting a broken site support issue
[ ] I've verified that I'm running youtube-dl version 2<IP_ADDRESS>

this is unrelated to youtube-dl program

[ ] I've checked that all provided URLs are alive and playable in a browser
[ ] I've checked that all URLs and arguments with special characters are properly quoted or escaped
[x] I've searched the bugtracker for similar bug reports including closed ones
[x] I've read bugs section in FAQ

Verbose log

$ flake8 .
./youtube_dl/extractor/youtube.py:2613:5: E122 continuation line missing indentation or outdented
./youtube_dl/extractor/youtube.py:2614:5: E122 continuation line missing indentation or outdented
./youtube_dl/extractor/youtube.py:2615:5: E122 continuation line missing indentation or outdented
./youtube_dl/extractor/youtube.py:2616:5: E122 continuation line missing indentation or outdented
./youtube_dl/extractor/youtube.py:2617:5: E122 continuation line missing indentation or outdented

Description

While merging the code to my fork, I noticed that flake8 fails after this change: https://github.com/ytdl-org/youtube-dl/commit/82abc13aedb42d77afcb0ca9c1d7982955826b66
This itself is still serious, but I notificed email from Travis says "Welcome to Travis CI" "You have 10,000 credits left".
I found out that Travis CI's pricing has changed, and ytdl-org is in trouble.
And it brought a more serious problem: No flake8 test on any PRs after the pricing change. Tests in PRs are not working at all.
Reconfiguring Travis CI may be needed.

We're out of credits and already requested more OSS credits.


sort_custom() giving error and (heavy) lag...

2 days ago I decided to update/recompile 3.1 Godot.
Now sort_custom() is giving me an error and unplayable heavy lag because I'm using it in _process(delta)
The error message:

ERROR: Array::sort_custom: Condition ' !p_obj->has_method(p_function) ' is true. returned: *this
At: core\array.cpp:261

I checked the array.cpp line 261 and it was updated 3 days ago: https://github.com/godotengine/godot/commit/6415454581c3ba0025da6b9bae42e060fa4e1508
As someone who is not a C++ programmer I've no idea how to work around it. Did the behavior of sort_custom() changed or is it a bug?
And if it's a bug, is it save to recompile Godot again without the newly added Array.cpp's 261 line?
Godot version:

Godot 3.1 alpha (non-official), commit 3bcab46ea513aea363951ec7301e80e262408970
OS/device including version:
Windows 10
Issue description:

I expect sort_custom array to just do its thing... now I'm unable to stop the lagging and error in the Array.cpp (line 261)
Steps to reproduce:
Use in GDscript the sort_custom() function and it will give an error. Pointing towards the Array.cpp file.

Judging from the error I would say that you pass a nonexistent function to sort_custom. Please make sure you didn't make a typo.
But this does not explain the lag. Maybe some other commit is causing it.
Or you try to sort and/or print a very big array every frame.
But without code I can not do more than to guess.

CC @DualMatrix

@DavidSichma
No typo involved, because the function name is "sort", so, not so much room for possible misspell.
Did you tested the sort_custom() function in the latest build? No errors for you? If so, how?
For me sort_custom() worked fine in my project since Godot 2.1.4-ish, 3.0, and early 3.1. No error, no lags while still using same similar size arrays.
Only after recompiling Godot from latest master branch 2days ago it stopped working properly, getting the specific error and massive lag inside _process(delta). Haven't touched sort_custom logic.

Can you please send me the script to take a closer look? Because sort_custom works fine for me.

@DavidSichma
Here is just very simple usage of sort_custom().
It works fine in previous versions of Godot, but not recent commit.
extends Node2D

func _ready():
	var y= []
	y.insert(0, 2222)
	y.insert(1, 3333)
	y.sort_custom(MyCustomSorter, "lol")
	pass

class MyCustomSorter:
	static func lol(a, b):
		print("ok doing something")
		if(a > b):
			return true
		else:
			return false
		pass

I think I located the issue.
You are passing an inner class with a static function. Commit 6415454 added a check to make sure the object you pass actually has this method. But it turns out that has_method() doesn't actually work for classes and static methods.
has_method() will always return false in this case resulting in the test failing although it should just work.
This should be a seperate issue.

For now you could try to use non static functions and instance the class or adding the method to the main class and passing self. Or, if you really want to do it, remove the error checking line.

@DavidSichma
I don't use the static function in my original code, but both non-static and static giving me the same error.
what do you actually mean by adding to the main class?
extends Node2D

func _ready():
	var y= []
	y.insert(0, 2222)
	y.insert(1, 3333)
	y.sort_custom(self, "lol")
	pass

func lol(a, b):
     print("ok doing something")
		if(a > b):
			return true
		else:
			return false
		pass

Yes, this is what I meant.
Just fix the the indent and it should work.

I just encountered the same issue. The workaround of instancing the inner class and not using static classes works, however the docs suggest a static approach (http://docs.godotengine.org/en/latest/classes/class_array.html#class-array-sort-custom), therefore it's confusing for the users.
I'd say revert https://github.com/godotengine/godot/pull/22737/commits/6415454581c3ba0025da6b9bae42e060fa4e1508 for now and reopen the original issue so that a better solution can be made.
CC @akien-mga who merged the original PR.

The true issue seems to be the has_method function being wrong. So as soon as that is fixed the error will dissapear and according to @DavidSichma in #22838 the fix for that is trivial.

I have barely tested my fix. It is a miracle at best this even works. I also just found out that this fix is not free of bugs.

I've reverted 6415454 for now with c730957, as the fix for #22838 doesn't seem that trivial in the end. The patch could be readded (as it was correct per se) once we have a solution for #22838.


PhP 7.3 compatibility

Hi,
recently our project switched to PhP7.3 and today we ran into some issues with our pipelines running phpqa. The problem is that the image uses PhP7.2 which does not support some syntax introduced in 7.3 (e.g. a trailing comma in a function call).
Because of this the parallel-lint task fails. Any suggestions for this problem? Is there a plan for upgrading to newer PhP version?
Thanks.

Custom docker image is solution, please see links in https://github.com/EdgedesignCZ/phpqa/issues/199#issuecomment-590258608.


Destroying an object via despawn leads to a warning message

Describe the bug
Despawning an object leads to warning message "Trying to destroy object N, but it doesn't seem to exist anymore!" Note this happens when running as Host, even if there are no other players connected. Other than the warning message the object appears to be destroyed correctly and propagated to any connected clients.
To Reproduce
Create a prefab with box collider set as trigger and add a script with the following code. Note the issue doesn't appear limited to this scenario, it was just the simplest example I could think of.
private void OnTriggerEnter(Collider other)
{
    if (IsServer)
    {
        var obj = this.GetComponent<NetworkObject>();
        obj.Despawn(true); // After this, a warning message will be generated.
    }
}

To spawn the object, create an empty game object and associate the following script. Point "target" at the prefab above.
public NetworkObject target;
void Start()
{
    if (IsServer)
    {
        var o = Instantiate(target, this.transform.position, this.transform.rotation);
        o.Spawn();
    }
}

Play as Host. Walk into the spawned object.
Expected behavior
On collision with the spawned object, it should be despawned without logging a warning.
Screenshots
N/A
Environment (please complete the following information):

OS: Windows 10
Unity Version: 2020.3.0f1
MLAPI Version: 0.1.0
MLAPI Commit: 17bd894dff

Thanks,
Carl

I can't reproduce this with the provided steps & code (the box is successfully spawned, the OnTriggerEnter code successfully executes, the object is destroyed and removed from the scene, but no console output is observed).
Can you please provide a minimal sample project that reproduces this error?

Thanks Jesse. Example project attached below. I stripped out everything except the scripts I mentioned above. StartHost is now called in the spawning script.
Cheers,
Carl
mlapiDespawnIssue.zip

Here are some screenshots of what I see. Before collision:

After collision:

I'm not able to repro with your sample on macOS and Unity 2019.4.19f1, 2020.2.7f1, or 2020.3.1f1.
I'll mark this for triage again and we can get it scheduled for someone to look at with a Windows 10 machine in case that's somehow related.

Thanks Jesse. I just tried again with 2020.3.1f1 and still see the issue, so looks like it isn't the Unity version but it could be Windows-related.

Noel is going to try and repo this on Windows

Hi Skep71c,
I have replicated this warning message on my side.  This is was a known issue and there is currently a temporary fix to prevent this message from being display in our development branch.
If you wanted to implement this fix locally you could replace the NetworkObject.OnDestroy method with this:
    private void OnDestroy()
    {
        if (NetworkManager.Singleton != null && NetworkSpawnManager.SpawnedObjects.ContainsKey(NetworkObjectId))
        {
            NetworkSpawnManager.OnDestroyObject(NetworkObjectId, false);
        }
    }

@JesseOlmer @will-mearns
The reference Jira bugs:
MTT-556: Remove Spam Generating NetworkObject.OnDestroy Sequences
MTT-557: Improved NetworkLog
Between the above two Jira entries, this visual bug will go away.

Great, thanks for letting me know Noel.

Yeah, it is annoying... that issue should go away come V1.0.0, adding that change to the OnDestroy method should make the spam stop for you.
@will-mearns This was a duplicate bug and is being tracked in MTT-556.
(An immediate fix is in place currently but will be further reviewed during our scene object spawning work)

Awesome, thanks for updating the info/links

Hi @sk3p71c,
I have upgraded your sample project to work with the new Netcode for GameObjects:

I had to make an adjustment in your scripts to replace the using MLAPI namespace with using Unity.Netcode, and the Spawner.cs:
using Unity.Netcode;

public class Spawner : NetworkBehaviour
{
    public NetworkObject target;
    void Start()
    {
        NetworkManager.Singleton.StartHost();

    }

    public override void OnNetworkDespawn()
    {
        if (IsServer)
        {
            var o = Instantiate(target, this.transform.position, this.transform.rotation);
            o.Spawn();
        }
        base.OnNetworkDespawn();
    }
}

Running your sample yields no warning messages and the cube spawns and falls on top of the host-player's object.

Cheers,
Noel