feat(dialog): Specify a desired aspect ratio to improve responsiveness

Bug, feature request, or proposal:
I have a dialog that is too narrow on large screens if I don't specify a minWidth. But if I specify a minWidth, the dialog is too wide. Since the dialog's minWidth can be specified in the dialog config, it does not feel right to specify it in CSS, but with CSS I can use media queries.
What is the expected behavior?
Actually it would be best if MatDialog could avoid strange aspect ratios by itself. The dialog config could be extended by an aspect ratio property and MatDialog tries to stick to this aspect ration while making best use of the available space. This way I could specify a maxWidth and an aspect ratio and the dialog would look good on all resolutions.
What is the current behavior?
Without minWidth the dialog becomes too narrow on large screens. With minWidth I get an unnecessary horizontal scroll bar on small screens. I can use CSS, but this seems a bit wrong as there is specific property for this purpose.
What are the steps to reproduce?
The problem appears if you have a dialog with let's say 5 input field rows and a 6th row with the Save/Cancel buttons. Without a minimum width the dialog's aspect ration on large screens becomes too narrow. With a minimum width I can make the dialog look good on large screens, but then it will be too large for small screens.
What is the use-case or motivation for changing an existing behavior?
I want dialogs to look good on all screen resolutions.
Which versions of Angular, Material, OS, TypeScript, browsers are affected?
Angular 5, Material 5rc2, all major OS, Typescript 2.4, all major browsers.
Is there anything else we should know?

It's slightly counter-intuitive, but if you set the 'width' to some pixel value (e.g. 500px, 1024px) instead of trying to set 'minWidth', it might do something close what you want. The 'width' configuration acts more like a max width in practice, since the dialog will still take on a reasonable widths even when the viewport has insufficient width available. Then you can use 'maxWidth' dialog config setting to essentially control the gutter for while the viewport width is less than the 'width' value. e.g. setting 'maxWidth' to '100vw' causes there to be no gutter at all for viewport widths less than the 'width' setting. The default 'maxWidth' value of '80vw' causes some gutter to be present even at small widths.

I think with the new aspect-ratio CSS property having good support it should be possible to solve this quite easy?

Responsive behavior for the dialog is best implemented using the minWidth and maxWidth properties. I don't think that introducing an aspect ratio is going to help much, because on a narrow screen the height is going to become even narrower in order to maintain the aspect ratio.

Well. But actually I often have dialogs that tend to be extremly "flat" (low height) and on wide screens. For example if there is very few content. So I would like to have a minWidth for small devices but no maxWidth as the dialog might have a lot of content, but if it has only little content it should not use the maxWidth but use some height.


Add mode for "Closure Stylesheets (GSS)"

The only difference from standard CSS so far is that it parses syntax properly inside @component blocks and also doesn't indent them.
Fixes #3470.

Merged as e79d4ffafb3300 . Though I'm somewhat confused why you went to the effort of adding a test without making sure it actually passed. Fixed that in 5d853959ca8a

Weird. I did some last-minute refactors, but thought I verified tests pass after those. Which ones failed? (Travis results look broken for a different reason.)

For one thing, the getMode call didn't actually name the mode correctly.

Yeah, I moved the test file and changed the name and at least thought I re-ran tests. No idea about the other change preventing negative indent, though. Didn't see any failures at all and I think that one would have shown up before my last-minute changes.
Anyway, sorry for test failures. I'll do better next time. =)

How often do you do version bumps? It looks like there hasn't been a new version since 5.6.1, before this change.

Once per month. 5.7 is coming out early next week.


Nexus Mods no longer bypassing

Domain
https://www.nexusmods.com/
Link
https://www.nexusmods.com/skyrimspecialedition/mods/77779?tab=files&file_id=331352&nmm=1
Version
0.0.0
What browsers are you seeing the problem on?
Firefox
What OS are you seeing the problem on?
Windows
Anything else?
Doesnt work on Chrome either.

still an issue

Still an issue, actually got worse now

unstale


Backdoor to defeat MPS backend

Description
PyTorch's MPS backend is a WIP: https://github.com/pytorch/pytorch/issues/77764
Thus, let's add a backdoor to use CPU backend over MPS for macOS
Type of Change

[x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

[x] Added new unit/integration tests
[ ] Added new notebook (that tests end-to-end)
[x] I stared at the code and made sure it makes sense

Hey @jamesbraza , curious why you think this is necessary? In most places where infer_torch_device is used, we already allow user to override with device to use, so you can just override to use cpu there

That's a good point, I made this because I needed a way of affecting the inference for models that MPS doesn't working for yet.
You're right, maybe the caller should just specify, instead of tweaking inference algorithm.  Sold me, gonna close this out


thread 'main' panicked at src/main.rs:130:10:

thread 'main' panicked at src/main.rs:130:10:
Failed to start command: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

you need to install tofi or specify another launcher in the config

you need to install tofi or specify another launcher in the config

i have wofi

so see the Configuration section in the README and configure it to use to wofi :)


Hoist common dependencies to the workspace, remove unused ones

When Cheatnet separated from Forge some unused and shared dependencies were left in the Cargo.toml files.

I will like to handle this issue.

Hi again, I am literally a green-horn when it comes to oss. Can you please provide more details on how you want the issue fix and specifically what needs to be done?


"Play" Tab: Game info view not being updated when lobby closed

The problem seems to be something like missing "lobby closed" event handling. Maybe it's intended behaviour though, not sure.

Create some lobby.
Click on it in FAF client, so that lobby info info would be shown at the right side.
Abandon the game (or launch it), lobby should disappear from the list of games.
Despite lobby no longer being in the list, it's info would still be shown at the right side.

Example: game "800 1500" displayed at the right side was already finished when the screenshot was taken:

Yes we should remove it when the game is no longer in the list.

No wait that is not fixed by this ups


Not building for android and iOS

Android output:

iOS Output for platform :ios, '13.0'

Are you using the beta version? This version is currently only supported by the latest flutter release because of a missing import. I will see if i can get it back for the next release but for now i recommend updating to latest flutter or reverting back to version 2.0.0

Thanks.. it worked with latest flutter version...

@juliansteenbakker In this case, you can use the flutter constrains in the pubspec.yaml like:
environment:
  sdk: ...
  flutter: "=< 3.3.3"


Revert minor part of #16968 to fix message redelivery

Fixes https://github.com/apache/pulsar/issues/17028 https://github.com/apache/pulsar/issues/16830
Motivation
In #16968, there was a change to remove some logic that appears necessary for certain message delivery guarantees. This PR fixes the minor regression introduced in that PR.
Modifications

Set sendInProgress = true before dispatching the read request.
Remove volatile keyword from sendInProgress. It is only updated within synchronized blocks.

Verifying this change
I verified that both testDelayedDeliveryWithMultipleConcurrentReadEntries and testMessageRedelivery pass consistently. Both were failing on my machine and stopped failing when I set sendInProgress = true.
Documentation

[x] doc-not-needed

I removed the synchronized block because the two method calls are independent and do not need to happen with in the same lock (as far as I can tell).

Hi, this problem has already reverted at PR #17018

Thanks for pointing that out @mattisonchao. Closing this as it is superseded by #17018


Be more friendly to third party users

I would like this page to be a page that third party libraries can link to and all necessary information for their users is on that page.
This page should also be written in a very basic way.

:+1:
can you please check spelling? i think we write PHP and HTTP and use the us-english organization instead of organisation. zend likely needs to be added to the word list.

Thank you. I've fixed the spelling now

Is this ready to me merged?

👍 from me

much better. i will try to send another iteration to hopefully further improve it

Awesome. Thank you for merging


Remove 'Infinite approval' setting

The value of this setting was used for AMM pools, but now that pools have been deprecated, this is not needed anymore.

[ ] Remove the option from the settings dropdown
[ ] Remove any reference to the infinite approval variable

Closed by https://github.com/oceanprotocol/market/pull/1857


wasm-filter-fix-and-remove-redundancy-in-categories

Description
This PR fixes #1304

removes redundancy in categories ( like Deployment, deployment)
list wasm filter in Technology section
wasm filters are now accessible at catalog/webassembly

Notes for Reviewers
Signed commits

[ ] Yes, I signed my commits.

Will the other category types be available when their respective designs will be published or do we need to specify them manually?

@vishalvivekm, thanks for this, keep the filter icon style consistent in dark mode, please

I'm afraid we don't have dark mode compatible logo for webassembly @thisiskaransgit

Will the other category types be available when their respective designs will be published or do we need to specify them manually?

Yes @thisiskaransgit , as the catalog is updated and new patterns are added, we get the type and compatibility values and then create the unique list of categories and technologies out of 'em. Thanks to @GaganpreetKaurKalsi :))

The lowercasing of "Deployment" and all of the other catalog "types", @GaganpreetKaurKalsi, is causing duplicative categories to be published.

Why is "All" capitalized, but not the rest?


[i18n] Cannot Handle Chinese

>>> WriteLine("中文")
??
>>>

Even with:
>>> using System.Text;
>>> Console.OutputEncoding = Encoding.Unicode
>>> WriteLine("中文")
??
>>>

Might be a Roslyn problem. Has nothing to do with <SatelliteResourceLanguages>en-US</SatelliteResourceLanguages>
See also: https://github.com/dotnet/roslyn/issues/4255
Because of this, when using console redirection: pure MyScript.cs > Output.txt, the output will be messed up and filled with ??? for Chinese characters.

Notice also this behavior:
>>> WriteLine('\u2103')
?
>>> using System.Text;
>>> Console.OutputEncoding = System.Text.Encoding.UTF8;
>>> WriteLine('\u2103')
℃
>>> WriteLine("中文")
??

Notice that the same issue DOES NOT exist when we simply write to file: File.WriteAllText("Output.txt", "中文"). For that reason, I think that's more of a standard output problem.


Zulip doesn't install curl

Running upgrade-zulip-from-git master produces this error:
Downloading tarball...
/home/zulip/deployments/2019-04-25-00-44-49/scripts/lib/../../scripts/lib/third/install-yarn.sh: line 43: curl: command not found
> Failed to download https://yarnpkg.com/downloads/1.15.2/yarn-v1.15.2.tar.gz.
Traceback (most recent call last):

Running apt install curl and re-running the upgrade works.  From the other recent issues I had it looks like many dependencies of Zulip are not made requirements (unless the new docs added this).

@stone212 I think it must have been removed on your system; scripts/setup/install installs curl as one of the first things it does.


Add automatic MAL udpate

It will be great to have 9anime companion update mal automatically like what Essentials for KissAnime or Masterani.me is doing.

Hi @Vardex ,
Thanks for the suggestion. I will definitely try and see if this is doable. I will keep you posted about the updates here. (Might take time since I am free only during the weekends.)

There's another extension for a different anime website called " Essentials for Kissanime "
It has already implemented Automatic MAL Update, just putting this here so that you could use it as a reference.
https://chrome.google.com/webstore/detail/essentials-for-kissanime/jaicfjhjlblbkhejmkfimkafjchdnafm

@FunkyJustin I have already finished the ground work for it in the rewrite. I will add this very soon. It will be added in 1.0.0-beta.2


Replace nonce implementation of BitcoinToYou and Mercado Bitcoin to the generic ones

To BitcoinToYou it can be replaced by https://github.com/timmolter/XChange/blob/develop/xchange-core/src/main/java/com/xeiam/xchange/utils/nonce/LongTimeNonceFactory.java
The rule of BitcoinToYou is: any number, but always a number bigger than the last one. The millis is suitable because although limiting to 1 call / milli, the API does not recommend less than one call / sec.
The Mercado Bitcoin also can use LongTimeNonceFactory.java, but it have to be divided by 1000 because the rule of Mercado Bitcoin is Unix Epoch Time in seconds. I am not sure if IntTimeNonceFactory.java can be used.

Done.


Prefer fixed-width integer types over fundamental types like unsigned int.

Subject of the issue
Many interfaces and class members use unsigned int. This type was seemingly picked as a C++03 shorthand for std::size_t or at least an approximation for std::uint32_t even though unsigned int isn't necessarily 32 bits wide. If our intent is to use 32 bit (or 64 bit) unsigned integers, let's use a type which better expresses and guarantees our intent.
A similar argument could be used for any uses of short, int, or long although are not nearly as pervasive in the interface so I'm not necessarily calling those out.
See this cppreference page for more information on the various possible widths of integral types.

https://github.com/ChrisThrasher/SFML/commit/fbe8fed83164561973389505d66396c16822cf8e
Starting off with unsigned short was easy so I went ahead and did that to help illustrate my point. Luckily unsigned short is always 16 bits so this one is unlikely if not impossible to have caused any bugs but I still think it's worth the added readability to know that we're working with 16 bit ints.

I want to help with this. Is there any guideline in which cases to use which types? Let's say there is an unsigned long is it safe to assume it would likely be 32-bit?

We haven't yet made a decision about whether or not we want to do this so there is as of yet no work to do.

I've thought about this more and think that focusing on changing unsigned int to std::size_t in APIs is the most valuable change to make. C++ programmers are so used to seeing std::size_t as the type for quantities of things. SFML would benefit from following that pattern.
Beyond that I think the marginal value diminishes quickly so I probably won't push on this issue much farther.

Looking a bit around, it seems that if one doesn't have a reason for a specific fixed-width integer, it's more recommend to just stick with int. So I wouldn't really want to convert everything to a fixed-width integer, only if we can be specific as to why it should be 32-bits or 64-bits etc.
For indexes and "countable" things, I think it makes sense to prefer std::size_t over unsigned int.

The JSF Coding Guidelines written by C++'s inventor Bjarne Stroustrop recommended as far back as 2005 to avoid using variable-length integer types and provides some argument for doing so. This makes me feel even more confident that we don't need to keep using things like unsigned int going forward even when we're not dealing with counts of things. It's still not massively valuable to make this change but it would an improvement if we did.

I wouldn't put too much weight on just one guideline, especially if it was for a very specific industry (jet fighters) and predates C++11 by a lot, even if it's "from the C++'s inventor".
There are plenty of other guidelines out there that go in other directions. It's always easy to pick one example and argue why it's the "correct" source of truth. In the end our decision should be based on our needs/targets and not just because someone defined it for their objectives.
IMHO, as long as we don't have a reason for a fixed-width integer type, it's better to let the compiler pick the actual type and it can be argued would also be the "most efficient" type for that architecture.

Closing due to lack of enthusiasm from the team. I still think this issue has value but we need to triage for v3 and this isn't important enough to make the cut. Maybe in v4 we can reconsider our use of variable-width ints in our APIs.


Change ioc.yaml to e.g. bl38p-mo-panda-01.yaml and provide access to the filename prefix Jinja renders.

It would be great to be able to add a description:  field to the ioc yaml that would be used to generate a $(P):DESCRIPTION PV inside the IOC. This could be used to populate hardware status and for scripts summarising IOCs on a beamline.

@GDYendell where would $(P) come from? None, or one or more of the templates in the IOC might have a $P.

Perhaps the prefix could come from $IOC from the deviocstats template.

Yeah I think it should be the name of the IOC. Will there always be a devIOCStats by default, without the beamline engineer having to define it manually?

Thats a good question. At present no. But this conversation plus other considerations are making me think that this is a good idea:

have a yaml entry that specifies the IOC prefix for devIOCstats (or just use the name field that we already have - but this means constraining that field to legal PV prefixes)
automatically instantiate devIOCstats using that prefix

@coretl do you have any views on this?

I have a slight preference for no magic, but also want to avoid copy paste errors. How about:
# Contents of BLxxI-MO-IOC-01.ibek.ioc.yaml
generic_ioc: ...
entities:
  - type: devIocStat.iocStats
    P: {{ yaml_prefix }}
    description: All endstation motors
  - type: pmac.pmac
  ...

where yaml_prefix is passed into the jinja context with value "yaml_filename.basename.split(".")[0]`
If we do this then we need the top level ioc_name and description for anything?

Just want to say: We need this.
At present I'm writing the name of the IOC in three places:

the folder name of the IOC
twice in the YAML

# yaml-language-server: $schema=https://github.com/epics-containers/ioc-pmac/releases/download/2023.11.1/ibek.ioc.schema.json

ioc_name: bl20j-mo-brick-01
description: geobrick 01

entities:
  - type: devIocStats.iocAdminSoft
    IOC: bl20j-mo-brick-01

At the moment we seem to have both lowercase and uppercase versions of the EPICS prefix. Can we standardise on the uppercase version? (as devIocStats will require the uppercase to make the right PVs).
How about we have BLxxI-MO-BRICK-01/ioc.yaml which uses {{ yaml_dir_name }} whenever it needs the string BLxxI-MO-BRICK-01?

@coretl
I disallow uppercase names because helm will not accept them.
https://epics-containers.github.io/main/user/tutorials/create_beamline.html#create-a-beamline-repository
https://helm.sh/docs/chart_best_practices/conventions/#chart-names
I also disallow them because they are a pain in the arse to type and because they look like they come from the 80's computer era and are an offence to cloud native tools. :-)
I think I'm OK with PV names are always ucase and kubernetes names are always lcase. If we have that as a rule then we can translate where required.

So I like your proposal but we need
{{ yaml_dir_name_upper }} and  {{ yaml_dir_name_lower }}

@coretl if you like this I'll do it next because it's quick but invasive.

I propose using https://jinja.palletsprojects.com/en/2.11.x/templates/#upper, so it would be {{ yaml_dir_name | upper }}. It looks like helm can do the same thing https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#pipelines. If helm has a different name to yaml_dir_name then we should use that instead...

Ah.
At all times that the ioc.yaml is being rendered it has been symlinked or copied into a generic folder name. This is particularly true at runtime where ioc.yaml is an entry in a config map and its path is just /epics/ioc/config/ioc.yaml.
So we could contrive to pass the folder name around to all the right places OR we could make ioc_name:  in the root of ioc.yaml the source of truth.
This does still mean repetition of the name in folder name and file header. Although the folder name does not have to be the same anymore it still makes sense to have them match. And that rather grates on me as its fiddly to maintain and could easily confuse.
more thought required ...

After a discussion with @coretl we decided the best course of action is to have named files. e.g. ioc.yaml --> bl38p-mo-panda-01.yaml and use {{ yaml_ioc_filename | upper }} for iocStats IOC field.
However this is a breaking change. We also decided to collect all breaking changes up for a single sprint in a few months time after multiple people have tried out epics-containers. Hence https://github.com/orgs/epics-containers/projects/1

I have updated ibek to support the named ioc.yaml file in future here 52233540d65.
The approach is to have two additional global names in the Jinja context.

ioc_yaml_file_name
ioc_name

ioc_name is rendered at the beginning of ibek runtime commands so that it may also include ioc_yaml_file_name.  In turn  ioc_yaml_file_name holds the stem of the yaml file name.
This means you can write an instance like this:
# yaml-language-server: $schema=https://github.com/epics-containers/ioc-adaravis/releases/download/2023.11.2/ibek.ioc.schema.json

ioc_name: "{{ ioc_yaml_file_name }}"
description: camera for diagnostic 2

entities:
  - type: devIocStats.iocAdminSoft
    IOC: "{{ ioc_name | upper }}"

This would give you IOC for you iocStatus prefix at present because every ioc instance has ioc.yaml filename but you can instead just write:
ioc_name: bl16i-mo-ioc-12
...

In this way we can for now have fixed names in ioc_name for now and then replace with {{ ioc_yaml_file_name }} once we change the filenames from ioc.yaml.
I then propose leaving the ibek code as is because it gives the ability to tweak your IOC name with suffixes or not use the filename at all if you want for some reason.
The alternative is we ditch ioc_name altogether an use ioc_yaml_file_name directly in the entity args. I slightly prefer the former as it gives you one place to supply a modified ioc_name and then re-use it multiple times.
@coretl does that sound OK?

While we are doing this we may as well change the name of the binary to the ioc name too. At present the binary is 'ioc'.
I just realized that all iocs look the same when running top on a server (although I was only doing this due to a hybrid server - in theory we should not care when everything is wrapped in pods and we only use K8S monitoring tools)

https://github.com/epics-containers/ibek/pull/147 has partially implemented this fix.
Completing the issue still requires:
updating ec to handle any stem for ioc.yaml
updating the tutorials to encourage unique naming of the ioc yaml
making sure that liveness.sh is pickiing up the same stem name (somehow - this uses helm chart name at present)

making sure that liveness.sh is pickiing up the same stem name (somehow - this uses helm chart name at present)

How about we add a new section to devIocStats.ibek.support.yaml:
- pre_init: ...
- liveness:
  - type: pv_connected
    pv: {{ IOC }}:UPTIME

Then we make the standard liveness.sh check all the PVs listed in instances of that YAML file. We can then do things like check the "Camserver disconnected: reboot IOC" source PV for Pilatus and automatically reboot the IOC in that state...

Really like that. It's even a workaround for https://github.com/epics-containers/epics-containers-cli/issues/57

This commit enables epics-containers-cli to accept any stem name for ioc.yaml b14bb34131dce42cc074d4f8b95912049ba504aa.
I'm pleased to note that the only change was for the ioc verify function.
But this did uncover a new issue https://github.com/epics-containers/epics-containers-cli/issues/64

What now remains to do on this Issue:

update the tutorials to create an appropriate name in the examples instead of ioc.yaml and make use of "{{ ioc_yaml_file_name }}" plus "{{ioc_name | upper }}
Add the liveness probe generator from ioc yaml

For the liveness probe I think we need a new ibek function ibek ioc generate-liveness and for the default start.sh to call it and put liveness.sh in /epics/runtime. It should also have a sensible default fallback for IOC instances that fail to supply the setting (even if that is a no-op)

I implemented this a while back.
Then I un-implemented it.
We now expect ioc.yaml (but accept other names) and define the name of the IOC in IOC_NAME environment variable set in the helm chart.


Required version invalid on main download link

Seems the version number isn't properly formatted on the main site link, should be an easy fix.
http://www.evernote.com/shard/s58/sh/b1231fd1-7cf8-4857-96a9-79fa0d8fcefb/85e25c0fcff51b81d5986d23bad05f45

mmm, mybad tampermonkey was disabled. All good


feat: add custom tls config support

Description of your changes

Fixes #
I have:

[ ] Read and followed Crossplane's contribution process.
[ ] Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Intended to create a PR on my local fork, but was not ready to submit PR here just yet, sorry about that! ;)


#319

6to5 replaced with Babel

Can you drop the version update?  We will update the version when we push to npm.

Done


Needs CI tests for Windows

Possibly using CircleCI for Windows beta, or AppVeyor.

@TooTallNate Our best option for now is to use AppVeyor. Circle CI it still does not stable release the support for Windows

We've been very successful with Azure Pipelines on Next.js:
https://azure.microsoft.com/en-us/services/devops/pipelines/
I think now-desktop also uses it.

Azure Pipelines looks promising, let's give it a try 👍
Two PRs for this:

Get build script compatible with windows (#12)
Update azure pipelines config for testing with windows image (#13)

@amio If we are going to use Azure pipeline I recommend deleting the add/appveyor-configuration branch. I see no case that continues to exist 👀

@MAPESO Yep, branch deleted. I also disabled appveyor webhook, no more alert from appveyor now 😎

@amio Great !! 😺


buildGoPackage: remove

Closes https://github.com/NixOS/nixpkgs/issues/318069
I reordered most of the subsection in the Go docs.
Things done

Built on platform(s)

[ ] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

Waiting for 24.11 branch-off.

can someone merge/ give their final approval?

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/splitting-the-nixpkgs-and-nixos-manuals-into-pages/55466/9


V1 refactor

Hi!
I had some free time so i cooked something: feature/simplified-requests.
This is how i would append the API key to the requested URL without exposing it everywhere.
I know there is a lot of changes that is because i am on Ubuntu so i couldn't build .NET Framework projects but i push a new commit soon to make it easier to see my actual changes.

I agree that there is a simpler way of hiding the api key no doubt, your pull request addresses this and that was my original plan. However in the context of a web app, the usage may differ. A web app may have all movie requests scoped to a client in which case we would want to give the developer the ability to use a client specific api key.
Hey,
The idea was to bake it in with the refactor so nobody is forced to use this feature, but its available if required. Adding it in after the fact would just mean more refactoring more breaking changes. Plus, I was planning on building something at a later date that could use this library with different api keys, not sure if that will ever happen 😄
The api factory is syntactic sugar, it can be injected into a controller or a service which requires access to several apis without needing a massive constructor, in fairness that can be removed.

Whats most important to me here is developer experience and clean code. If we removed the ability to provide a user specific api key it would make the code a lot simpler but also takes away a powerful feature.
Aside from KISS is there any other reason why we wouldn't keep the per user apikey feature in?

In fact, your changes have prompted an idea.

Yes this make sense. I don't have too much experience with C# web apps so there is a big chance that i didn't think some use cases here. If i am not mistaken C# web apps could share resources (variables) between multiple requests not like PHP. The current implementation could help remove the overhead to initialize some classes (TmdbEasyClientv3, HttpClient, is there anything else?), but in my opinion this won't be a bottleneck for a web app.
I read somewhere that developers should focus on writing readable code and after that fine tune the performance. If You decide to set the API key only in the TmdbEasyOptions for now but some point in the future support to set API key for each request You could still inherit from a class (like now) without breaking change (i think).

Setting the api key on the TmdbEasyOptions will be the default. Its easy to configure and easy for devs to understand and it follows a well known pattern.
The current implementation will indeed cut down on the initialisation of TmdbEasyClientv3 and also supports DI out of the box. HttpClient is a tricky one and needs to be treated with care. It has been built and optimised to be reused for the entire life of an application and in fact can lead to serious performance degradation if scoped to a request.

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly. see ms docs. for more info on this.
I ran a test with just 20 requests and it left about 20 sockets taken long after the requests had ended.

A better way is to use the IHttpClientFactory but this is only available in .Net Core and this is a .NET standard library.
As you mention sharing resources like the apikey across requests is no problem at all.
I 100% agree with this.

I read somewhere that developers should focus on writing readable code and after that fine tune the performance.

Your changes have prompted an idea and I think i can make the code even simpler and still allow a user api key per request without exposing it to each individual api method.
Thanks for the feedback, great to have you back.

My other idea woud be to remove the string language = "en" from the methods and just add a public string Language { get; set ; } to the TmdbEasyOptions and append the content for each request (i don't think TMDb will reject a request just because an endpoint doesn't have an optional language parameter.

HttpClient could be injected to the TmdbEasyClientv3 ctor so it will make it easier for developers to use only one even if there is more TmdbEasyClientv3 creater for some reason.
Glad if i could help :smile:

Regarding the language idea, this would imply that every request would be scoped to a single language. I don't think this is ideal, since an application with different users in different countries might want to query for different languages.
The TmdbEasyOptions are meant to be created once and set at application startup. These options wont change for the lifetime of the app. This is the typical MS configuration pattern which works well in almost all apps I have designed and built at work.

Yes, You are right about the language for web apps. But if i use it for only one language setting it only in for example in the TmdbEasyOptions and forget about it would be easier than passing it for each request.

This PR closes #18

@PoLaKoSz this commit c9e2d95 has all the necessary changes to address #18  and #19
In summary:

The request building has been completely separated from the client and api code
Users have the choice to easily set an apikey globally via TmdbEasyOptions
Users have the option to override the global api key by providing an api key to each individual api request
Users have the choice to easily set an default language globally  via TmdbEasyOptions
Users have the option to override the default language by providing a language to each individual api request
ApiFactory has been removed as unnecessary syntactic sugar
Request building has been made slightly more robust to avoid basic errors, this might need tweaking in the future if via usage we find any bugs
Removes all custom request classes to expose the exact properties that are required vs optional for each api method

Thanks for you feedback on this, hopefully this makes the developer experience a bit smoother

I like repos with this folder structure (i follow it myself):

|- src
|    |- TmdbEasy.csproj
|
|- tests
|    |- Unit
|    |   |- Unit.csproj
|    |
|    |- Integration
|        |- Integration.csproj
|
|- README.md

What do You think?
Namespaces: TmdbEasy.Tests.Unit and TmdbEasy.Tests.Integration?

I really like this Request builder approach, very neat! Personally i even make it simpler by removing TmdbEasyClientv3.GetBaseUrl(), TmdbEasyClientv3.GetApiKey() and TmdbEasyClientv3.GetDefaultLanguage() methods and pass the TmdbEasyOptions into the RequestHandler and Request ctor to decrease the parameters count (and remove the 3 mentioned TmdbEasyClientv3 methods).
Just to make the code even more readable (fluentish) i would rename the RequestHandler.ExecuteRequestAsync() to ExecuteAsync.
You sad that the TmdbEasyOptions should be set only once. The class does not indicate this (public setters). I know its bit more code but like i suggested before, set the properties in the ctor instead (i would also change the visibility of the properties to internal too).
I would simplify our life to this:

namespace TmdbEasy.Configurations
{
    public class TmdbEasyOptions
    {
        private const string TmdbUrl3 = "http://api.themoviedb.org/3";
        private const string TmdbUrl3Ssl = "https://api.themoviedb.org/3";

        public TmdbEasyOptions(bool useSsl, string apiKey, string defaultLanguage)
        {
            this.BaseUri = useSsl ? TmdbUrl3Ssl : TmdbUrl3;
            this.ApiKey = apiKey;
            this.DefaultLanguage = defaultLanguage;

        }

        internal string BaseUri { get; }

        internal string ApiKey { get; }

        internal string DefaultLanguage { get; }
    }
}

and now the GetBaseUrl() from TmdbEasyClientv3 could be removed too. I think specifying the BaseAddress for the HttpClient was unnecessary because the query contains the full path, am i right?

I see this everywhere but i just can't understand this Async suffix. I see when this naming convention could be usefull, here is an example, but i never used it because when i build upon a HttpClient every call will be async.

I edited your post with some numbering so it would make my comments easier to follow:

Yes I use this at work as well, tried something different here but I don't like it either
Agree this can be an easy way of simplifying the code
Yes that sounds good 👍
Typically you would hold on to that class after its been set at app startup so not really an issue, but I'm happy to have that change made.
Yes you are right, this can be removed if we make the change in 2.
There are varying opinions on this but I tend to follow the pattern of.

If a public method is Task-returning and is asynchronous in nature (as opposed to a method that is known to always execute synchronously to completion but still returns a Task for some reason), it should have an “Async” suffix.

I think Id prefer to leave 6. as is.
How are you doing for time? I still need to finish off a couple apis before merging v1. How about i start on 1. and 4. and leave 2. 3. 5. for you as they are closely related. If possible you could submit PR onto the v1 branch.
Let me know if you have time or else i can just knock those out. Thanks for the help

I code 2, 3 and 5 right now, and do the first after that because i think that will cause a merge conflict.

I missed your comment, I have made the changes to address 1.

Can You revert or hard reset the commit? Even GitHub says in #20 :smile: :

These conflicts are too complex to resolve in the web editor

I will revert after I have reviewed your PR. I will then apply my commits back on top of your PR once its merged

Thanks!

@PoLaKoSz thanks for help on this.
Releasing v1.0.0-alpha1 as we haven't got a full test suite for Movie and television api. Once that's done will bump to v1.0.0
@VPKSoft Will work on testing the television api within the week, I may have introduced a couple bugs.

Glad that i was able to help :)

Nothing stops the user to send a request without setting an API key (forgot to set in the Options and in the parameter too). Fix in #24

To send one request the setup is painfully long:
var deserializer = new NewtonSoftDeserializer();
var client = new TmdbEasyClientv3(deserializer, new TmdbEasyOptions());
var requestHandler = new RequestHandler(client);
var endPoint = new ConfigApi(requestHandler);
var jobs = await endPoint.GetJobsAsync();

Solution:

i would swap the order of the TmdbEasyClientv3 ctor parameters (i just want to supply an API key via Options and don't care how the library works internally)
then i would make the IJsonDeserializer an optional parameter
i would define clearly the responsibility of the ITmdbEasyClient interface: Provide an entry point for the library and nothing more:

public interface ITmdbEasyClient
{
    IChangesApi Changes { get; }
    ICollectionApi Collection { get; }
    ICompaniesApi Companies { get; }
    IConfigApi Config { get; }
    ICreditApi Credit { get; }
    IMovieApi Movie { get; }
    INetworksApi Networks { get; }
    IReviewApi Review { get; }
    ITelevisionApi Television { get; }
}

public class TmdbEasyClientv3 : ITmdbEasyClient
{
    private static readonly  string _tmdbUrl3 = "http://api.themoviedb.org/3/";
    private static readonly string _tmdbUrl3Ssl = "https://api.themoviedb.org/3/";

    public TmdbEasyClientv3(TmdbEasyOptions options)
        : this(options, new NewtonSoftDeserializer())
    {
    }

    public TmdbEasyClientv3(TmdbEasyOptions options, IJsonDeserializer jsonDeserializer)
    {
        if (options == null)
            throw new ArgumentNullException(nameof(options));

        var httpClient = new HttpClient()
        {
            BaseAddress = new Uri(options.UseSsl ? _tmdbUrl3Ssl : _tmdbUrl3)
        };

        var requestHandler = new RequestHandler(options, httpClient, jsonDeserializer);

        Changes = new ChangesApi(requestHandler);
        Collection = new CollectionApi(requestHandler);
        Companies = new CompaniesApi(requestHandler);
        Config = new ConfigApi(requestHandler);
        Credit = new CreditApi(requestHandler);
        Movie = new MovieApi(requestHandler);
        Networks = new NetworksApi(requestHandler);
        Review = new ReviewApi(requestHandler);
        Television = new TelevisionApi(requestHandler);
    }

    public IChangesApi Changes { get; }
    public ICollectionApi Collection { get; }
    public ICompaniesApi Companies { get; }
    public IConfigApi Config { get; }
    public ICreditApi Credit { get; }
    public IMovieApi Movie { get; }
    public INetworksApi Networks { get; }
    public IReviewApi Review { get; }
    public ITelevisionApi Television { get; }
}

i would change the visibility to internal for every class in the the TmdbEasy.Apis namespace (one less public namespace)
now we can make RequestHandler internal so we won't need the IRequestHandler interface anymore. I made a slight change in the RequestHandler ctor params: now it accepts 3 classes. This class could be responsible to send a request (this is why i moved the HttpClient here) to an URL and parse its content accordingly:

internal class RequestHandler
{
    private readonly HttpClient _httpClient;
    private readonly IJsonDeserializer _jsonDeserializer;
    private readonly TmdbEasyOptions _options;

    internal RequestHandler(
        TmdbEasyOptions options,
        HttpClient httpClient,
        IJsonDeserializer jsonDeserializer)
    {
        _httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
        _options = options;
        _jsonDeserializer = jsonDeserializer;
    }

    internal Request CreateRequest()
    {
        return new Request(_options);
    }

    internal async Task<TmdbEasyModel> ExecuteAsync<TmdbEasyModel>(Request request)
    {
        string url = request.GetUri();
        if (string.IsNullOrEmpty(url) || string.IsNullOrWhiteSpace(url))
            throw new ArgumentException("Generated url is invalid (null or empty)!", nameof(request));

        string json = await _httpClient.GetStringAsync(url)
            .ConfigureAwait(false);

        return _jsonDeserializer.DeserializeTo<TmdbEasyModel>(json);
    }
}

Result:
var tmdb = new TmdbEasyClientv3(new TmdbEasyOptions("apiKey"));
var jobs = await tmdb.Config.GetJobsAsync();

Yes, i removed the ApiVersion GetVersion(); method from the ITmdbEasyClient interface because we only support v3 for now so once again KISS principle :smile:.
RequestHandler ctor could be simplified probably with this:
public class TmdbEasyClientv3 : ITmdbEasyClientv3, ITmdbEasyClient {}

internal interface ITmdbEasyClient
{
    TmdbEasyOptions Options { get; }
    HttpClient HttpClient { get; }
    IJsonDeserializer JsonDeserializer { get; }
}

... and just pass this interface intead of 3 parameters (and probably the future TmdbEasyClientv4 could inherit from this interface too).

Hi Tom,

Thanks for the #24 just some failing tests to look at.
I am also OK with removing the version, not that it complicates anything but no problem there.

Regarding the rest, this refactor is far too large and in my opinion is not a SOLID design. I can see where you are coming from in that you want to have a simple API, however you are looking at this only from a desktop apps POV. The code changes made above would not play nicely with a web application and definitely not with Dependency Injection. see ms docs for a starter The client now depends on all the APIS as well.

There is clear violation of the SRP principle (The client is now doing too many things)

exposing APIS
new-ing classes up (taken them on as a dependency)
adds complexity when testing
cannot swap out the API implementations without changing the client
cannot swap out the API implementations at runtime either
creating HttpClient and RequestHandler per request as this would simple not be able to be shared across httpcontexts

Violates Open-closed principle -> each time we add a new API we have to change the client
2 constructors generally bad design (What are we telling the user with this)?

The above changes make it super easy for a desktop to spin up a client and make a call, but it makes things a whole lot more complicated and very messy for WebApps.
The API as it is, is very clear IMHO. It does not violate SRP, complies with Open-Closed and supports DI. It also decouples the implementation from the contract and makes it very easy for the dev to set things up as well as use, allows for API implementations to be replaced at runtime and without changing the client
If you look at .NET Core Config you'll see the setup is a one liner. If you look at Usage you'll see the usage is super simple as well. The DI container takes care of everything.
In the context of a desktop app, you may not have all the DI buts and this is where, instead of refactoring the whole library to suit a single use case, you would:

create a wrapper which would encapsulate your setup code into a single reusable method:

var deserializer = new NewtonSoftDeserializer();
var client = new TmdbEasyClientv3(deserializer, new TmdbEasyOptions());
var requestHandler = new RequestHandler(client);
var endPoint = new ConfigApi(requestHandler);
var jobs = await endPoint.GetJobsAsync();

create a factory that would return the types you need.

public IChangesApi Changes { get; }
public ICollectionApi Collection { get; }
public ICompaniesApi Companies { get; }
public IConfigApi Config { get; }
public ICreditApi Credit { get; }
public IMovieApi Movie { get; }
public INetworksApi Networks { get; }
public IReviewApi Review { get; }
public ITelevisionApi Television { get; }

In my opinion the above should happen within the client application(not within TmdbEasy) as it is extremely specific to that app.
is there a specific app that is using this library? Perhaps i can help you with looking at that. Typically desktops can make use of DI as well, but with the solution I mention above, it wouldn't have to.

Thank You for the detailed answer i really appreciate it. Yes, You are right, i have much less experience with web apps thats for sure but i have a few questions.
My understanding is that API key and the language could be set for each request, so this means TmdbEasyClientv3 can be registered as a singleton in the DI container. When the DI creates the TmdbEasyClientv3 it will initialize (currently 9) ...Api classes and it will live while the web server is running. When i typed my last sentece i realized that these 9 endpoints created inside the TmdbEasyClientv3 class so DI can't access them. ... But is is true?
Few hours later ... :smile:
After i rewrite completly (just a proof of concept) the TmdbEasyServiceCollectionExtensionsAddTmdbEasy()  method i was able to successfully run an ASP .NET Core WebAPI server and get result back from TMDb.

There is clear violation of the SRP principle (The client is now doing too many things)

You mean TmdbEasyClientv3? Its only responsible to initialize the library to be usable in the future.

exposing APIS

Yes, guides the developer how can access all endpoints without looking up the documentation.

new-ing classes up (taken them on as a dependency)

Yes, right now only 2 , and 9 required classes. But HttpClient should be injected through the ctor not as is now.

adds complexity when testing

What do You mean? You can test everything except the relative generated URLs but they covered with the Integrations tests (just not explicitly). In fact test setup even shorter.
I left the Request class untouched because URL generation is really important, but i would probably make that class internal too.

cannot swap out the API implementations without changing the client

I don't understand this point, sorry.

cannot swap out the API implementations at runtime either

I don't understand this point either, sorry. Can You give me an example?

creating HttpClient and RequestHandler per request as this would simple not be able to be shared across httpcontexts

HttpClient created inside the TmdbEasyClientv3 class only once in the ctor. It will be created once (even with ASP .NET Core because its registered as singleton).

Violates Open-closed principle -> each time we add a new API we have to change the client

Yes i agree with You. The same thing could happen when TMDb decide to add a new endpoint for example movie/{id}/actors. The person who will implement it probably add a new method to the IMovieApi interface so the principle will be break or is there any other option? Extension method?

2 constructors generally bad design (What are we telling the user with this)?

I never heard something like this. For me it means there are multiple good ways to initialize a class that is all.

No problem at all, its fun having discussions and looking at new approaches, learning new things etc.
Before I respond to questions, I'd like to mention singletons. Some people love them others hate them, I use them only if there is a compelling reason to.
I registered the TmdbEasyClient as singleton as it uses an HttpClient which needs to exist for the life of the application,so it make sense that this is a singleton. All the apis however, do not need to exist the entire life of the application.
In your sample, you have registered the TmdbEasyClient as a singleton and then all of its dependencies as well (9 apis)). So you have to ask yourself "do the api classes really need to live the life of the application?"
The answer is yes, because the design forces them too, they are unable to exist any other way. And this is an immediate flag that something might be wrong with the design. What if somebody only wants a movie-api, they have to live with the fact that there are 8 other apis new-ed up and alive for no reason at all.

There is clear violation of the SRP principle (The client is now doing too many things)

You mean TmdbEasyClientv3? Its only responsible to initialise the library to be usable in the future.
Looking at it again, perhaps it is only doing one thing.  If you have multiple constructors, then it may look like your object is doing different things, depending on how its constructed. And in this case it is. Probably a better way would be to have the JsonDeserializer an optional parameter. I still dont agree with what the class does or how it does it, but will comment on the rest of your questions.

exposing APIS

Yes, guides the developer how can access all endpoints without looking up the documentation.
This can be done via a factory, and id argue it should be done by a factory. I will see if I can get something working for you. On of the reason i am not hiding all these classes is to allow for them to be replaced by the caller, should the caller not like the way something is handled. This makes the library super flexible.

For example: My app might need to do something every time the RequestHandlers .GetUri() method is called (lets just say for the sake of simplicity), all Id need to do, is implement the IRequesthandler interface. i register it with DI and off I go, The library works without needing any changes.
PS. At the very least the dev should read the README file. There we have listed the 9 apis.

new-ing classes up (taken them on as a dependency)

Yes, right now only 2 , and 9 required classes. But HttpClient should be injected through the ctor not as is now.
This will only get more and more.  will discuss open-closed later on.

adds complexity when testing

What do You mean? You can test everything except the relative generated URLs but they covered with the Integrations tests (just not explicitly). In fact test setup even shorter.
Ignore this for now, Testing was referring to just the testing of the TmdbEasyClient class but after looking at it it might be ok.

cannot swap out the API implementations without changing the client

I don't understand this point, sorry.
I mntioned this above in point 2 as well. The user of the library might want to change the implementation of an API. With the current version they can do this very easily, since nothing in the library depends on the implementation(classes) of the APIS. So if I wanted to write my own NetworkAPI or RequestHandler or TmdbEasyClient and still have it work with all the rest of the library, I can. But with your changes, this would not be possible. The TmdbEasyClient depends directly on all the implementations.

cannot swap out the API implementations at runtime either

I don't understand this point either, sorry. Can You give me an example?
This follows on from 5. At runtime using a flag and DI we can change which version of an implementation we are using based on the user, the environment or anything else that we might need. You might say "but the library is super simple that will never happen" but that is besides the point, the design is decoupled and everything is programmed to an interfac, decoupled and interchangeable.

creating HttpClient and RequestHandler per request as this would simple not be able to be shared across httpcontexts

HttpClient created inside the TmdbEasyClientv3 class only once in the ctor. It will be created once (even with ASP .NET Core because its registered as singleton).
yeap, after you wrote a new serviceCollection extension method then I understood how it would be used.

Violates Open-closed principle -> each time we add a new API we have to change the client

You absolutly right. I don't think i eveer was able to follow the SOLID principles 100%, but i allways try as closly as possible. On the other hand we have to realize this library is a wrapper so we depend on an external dependency which could change any time. In my opinion declaring a clean entry point and addig new endpoints if necessary will go against the open-closed rule, but it would be a minor change and for me writing code as a developer is more fun then reading docs on how to use a library (even worse, when i have to read the source).
The goal is to use SOLID as a guide, so for sure we dont have to apply it 100%. Sometimes we just cant, but here we have a choice and we can follow the OpenClosed principle. The cass should be open for extension (add a new method) but closed for modification (not changing an existing method everytime we add a new feature(api in this instance)).

I still believe the best way to give you your siple developer experience is by leaving everything public as is so devs can change things if they wish. Then we can add a factory which can aid in providing the entry point you wish.
2 constructors generally bad design (What are we telling the user with this)?

I never heard something like this. For me it means there are multiple good ways to initialize a class that is all.
If you have multiple constructors, then it may look like your object is doing different things, depending on how its constructed. Generally, your object should only be doing 1 thing.

Maybe you can have a crack at implementing a factory on top of the master branch?? I will too as we can see what comes of it?

Thank You for taking the time i hope i am not full defensive here, i try to reason with as much objectiveness as possible :smile:
Given this code:
class Car {}
class Engine
{
      public static Engine Instance() { /* .. */ }
}

I never used singletons as described here and i think registering Engine class with DI as a singleton and calling a static method for an Engine instance is two different thing.
With singleton pattern developers hide the Car class dependencies because usually in the ctor will call _engine = Engine.Instance(); and this is a really bad practise.
On the other hand even if i register a class singleton with DI and pass the Engine dependency through the Car's ctor i won't hide the dependency so this is a good practise. If i register as singleton is just allocates less memory and when resolving the class second time DI, it will return the object faster.
You stated lot of time that You want to support web apps out of the box. Lets assume a website has an endpoint which uses this library to get a movie details and this endpoint called 10 times per second (this web app doesn't use anything to cache the responses so it will call TMDb every time.

If we register every I...Api as a singleton the DI will won't sweat because after the second request it will return the same MovieApi instance
If we register every I...Api as transient than the DI will resolve 20 new MovieApi instance in two seconds

You can see if request/seconds gets higher it gets ever worse for no good reason. No ...Api holds any user specifc state , thanks the option to set the API key and language on a per-request basis so no need to create each time we need a ...Api. And yes, You are right, in the memory there will be 8 unused objects, but compared to the 10 new object/second isn't it the best solution?

For example: My app might need to do something every time the RequestHandlers .GetUri() method is called (lets just say for the sake of simplicity), all Id need to do, is implement the IRequesthandler interface. i register it with DI and off I go, The library works without needing any changes.

All right, i can imagine that there is a use case to get every requested URL. You could expose IRequestInterface then as You mentioned the user have to create a new class, inject it, etc or You could expose an event on the TmdbEasyClientv3 (for example BeforeRequest where this event will return the generated URL from RequestHandlers .GetUri(). No new class, no injecting, no hussle just simple event subscription for the user. Only we have to implement it once internally without any breaking change and the need for any other library user who needs the same. What do You think about this?

Maybe you can have a crack at implementing a factory on top of the master branch?? I will too as we can see what comes of it?

Sorry, but i don't understand it, can You rephrase it for me, please? Thanks!

No problem at all, I dont take your questions and interest as defensive at all ;)
With regards to singletons, I am not referring to the singleton design pattern, as you mentioned that can be bad practice. What I am referring to is registering dependencies with DI as singletons only if you need to.
You should only really be registering singletons in DI for 2 reasons:

if the class directly depends on and uses resources which can be limited e.g. sockets, database connections etc
if state matters.

If i register as singleton is just allocates less memory and when resolving the class second time DI, it will return the object faster.

This should not be taken into consideration and should not be affecting your design. In most cases the performance hit cannot be noticed and is not an issue. We have several production web apps with hundreds of users and hundreds of services and there is no performance hit. A small ASP.NET Core kestrel server on a 1gb vm handles 1000's per second without even breaking a sweat.
Having 9 ...API instances alive even if we only need one is bad design but not because of memory resource. It is the concept of creating services when they are not needed that is wasteful.
 The doctor checks you out and says you need treatment. So he prescribes a fancy medicine that costs 1000£ a tablet and says you only need 1 tablet. So you go to the pharmacy with the prescription and the pharmacist gives you a box of 9 pills and says "unfortunately even though you only need 1 you have to buy 9, the other 8 will just have to sit around be there even if you don't need them". To some the cost wont matter, to others it will. In this example we are the pharmacist and its our job to give 1 pill when 1 is requested. We don't care about the cost, we care about doing correctness.
In the current version, the only class that needs resource that is limited(sockets) is the TmdbEasyClient since its the only class that really needs the HttpClient. From what I have gathered, I think you understand this, but let me know if not.
All the remaining classes depend on the ITmdbEasyClient interface and know nothing about the client. All of these classes do not need to be registered as singletons. In fact, it is best practice that they are not registered as singletons. Neither of them need access to a limited resource and neither of them need to maintain some sort of state.
Transcient and scoped service registration is generally better when the resource is used temporarily. In a typical web app, the user will make a request to the server, the server will spin up an ..API instance, do the work, once the request returns the server then disposes the instance(DI manages the lifetime of the instances).
Also you didn't comment on the fact that users may want to replace the ...API instance with their own versions or opt between versions. Generally your decision hides everything away and leave 0 room for extension or modification from the user.
Regarding the factory, I will get back to you on this with another comment, I need to pop out now.

Okey, i am gonna accept what You sad about registering singleton dependencies.
I forgot to write answer to this:

PS. At the very least the dev should read the README file. There we have listed the 9 apis.

Yes, they should be, but there could be a problem when the docs out of sync with the code. Intellisense hopefully never lies, so remembering only the TmdbEasy.TmdbEasyClientv3 class can help explore the whole library. Yesterday when i made the sample web app i encountered the same problem:
README.md
public void ConfigureServices(IServiceCollection services)
{
    // Adds TmdbEasy to your app
    services.AddTmdbEasy();
}

https://github.com/tonykaralis/TMdbEasy/blob/1b09de580f6dcb241904f4c9f6b551520b617890/src/TmdbEasyServiceCollectionExtensions.cs#L9-L12
The AddTmdbEasy has a required TmdbEasyOptions parameter.

Docs is another discussion. The README is just a simple bare bones intro to the Web App. I left the options out on purpose but you make a good point, I should probably include the options so the example is complete.
Trying to over simplify a library to save someone from going in to the docs is probably not a good thing to do. Think about most libraries, you need to at some point read something to figure out how things work, if you need advanced options etc.
Typically for any library most devs would go to the docs before using the library (at least this is what I advise for professional programming). In our case the library is small so we don't really have a  lot to put in the docs.
I will look at improving the README slightly but I will focus on keeping it short, simple and minimal. In theory, if I make a few tweaks to the Services section and add the options in, that should be enough for someone to get up and running.
Do you think a 1 pager wiki could be worth adding? I think its overkill.. whats your thoughts on that?

Interesting. I like using and writing libraries that are ready to use without any setup if possible. My opinion is when i want to access an API, i don't want to know how the library works i just want to call the correct method end get a correct response that i could use in my own app. Creating 2 additional (RequestHandler, ...Api) classes to do that, injecting into each other and then using the library is not what is in my mind (as a mainly ConsoleApp developer :smile:) ... 2-3 line top. If i am not satisfied with the library then i will read more about it. But this is my opinion, thank You for letting me know yours.
I think the README close to perfect and i don't think any Wiki page needed (i would disable the menu in the Settings too), only a small code sample for ConsoleApp devs missing in my opinion.
.NET Core Configuration section
- that dont use an IOC container.
+ that don't use an IOC container.

Just to keep the code sample short
public void ConfigureServices(IServiceCollection services)
{
-    services.Configure<CookiePolicyOptions>(options =>
-    {
-        options.CheckConsentNeeded = context => true;
-        options.MinimumSameSitePolicy = SameSiteMode.None;
-    });

    // Adds TmdbEasy to your app
     services.AddTmdbEasy();

-    // Other service registrations
-    services.AddAuthentication();

-    services.AddMvc();

-    services.AddAntiforgery();
}

The I...Api interface list under the Supported Services could be hrefs to the actual interfaces to easily navigate to each of them and take a look to all available members.
IUserService never used
public class MovieController : Controller
{
     private readonly IMovieApi _movieApi;
-    private readonly IUserService _userService;

-    public HomeController(IMovieApi movieApi, IUserService userService)
+    public HomeController(IMovieApi movieApi)
     // ...
}

Right Tom, I have been giving it some thought and understand your need to hide some of the implementation classes. I agree that we can trim this down but not by much.
I have the following suggestion:

The APIS must remain public and all the implementations as well. These need to be known, they need to be managed by the user, so new-ing them up should not be the responsibility of this library. We should not be responsible for the lifetime of these objects either. We can only advise what the suggested scope should be, offer a default and the rest is up to the user. If someone wants DI fine, and if a console dev doesn't want DI then also fine. We only give them what they absolutely need. That leaves us with 2 interfaces:

ITmdbEasyClient

IRequestHandler

IJsonSerializer

We absolutely need 1 one of these to be public, since we need a way to globally configure the library with options and a place to hold an HttpClient. We should therefore keep the TmdbEasyClient (class and interface) public and advise to register it as a singleton.

Hides this dependency. After some thought, i am not sure this class needs to be replaceable. We can allow for optional override via options

Hides this dependency, but allow an override via the options.

Changes made:

Updated README with your suggested changes see [977ec7baa6a6572ec7abc080bc239490e77b415b]
Renames TmdbEasyClient and removes version, see [d59a6445d83f5f8586d908220d011f607cec8a36]
Moved urls into Constants class. I did this as I was taught to keep constant values in a Constants file. Seems neater and more organised IMHO, see [24840de9a50e9083321ce35f354a0c606cf98781]
Adds default JsonSerializer, allows for overriding with own implementation and marks internal, see [9e9fa134d5dc4fe1047ba867769d56524d20209c]
Makes Request, Request

This means the following falls away:
// configuration
var deserializer = new NewtonSoftDeserializer();
var client = new TmdbEasyClientv3(deserializer, new TmdbEasyOptions());
var requestHandler = new RequestHandler(client);
// usage
var endPoint = new ConfigApi(requestHandler);
var jobs = await endPoint.GetJobsAsync();

and is replaced with this:
// configuration
var client = new TmdbEasyClientv3(new TmdbEasyOptions());
// usage
var endPoint = new ConfigApi(client);
var jobs = await endPoint.GetJobsAsync();

In all honesty I am not 200% sure if I want to merge the last change, hut will be the same

Yes, sounds way better. Moving the deserializer to the options class was a great idea. :smile:

FYI, I have released v1.0.0. Added tests for MovieApi and TelevisionApi. Listed a bug here #27. Have a good Sunday 👍

Cool, have some rest now :smile:


Advance find

Coverage decreased (-1.2%) to 90.171% when pulling bce46540310e8a1bcc1fc0e9b448f68f398f0f82 on advance-find into 36c8022bc0423b208b7533181b444d5f8952dc42 on development.

Coverage increased (+0.09%) to 91.491% when pulling 87f029adfffc6fe55ceaa1952523c95d6730cf58 on advance-find into 36c8022bc0423b208b7533181b444d5f8952dc42 on development.

Coverage increased (+0.09%) to 91.491% when pulling dca787d9adbd9cbbddf862748a945c4136be3b63 on advance-find into 36c8022bc0423b208b7533181b444d5f8952dc42 on development.

Coverage increased (+0.09%) to 91.491% when pulling 1fd242c190758d47ca4e771880372e5e596ab876 on advance-find into 36c8022bc0423b208b7533181b444d5f8952dc42 on development.

Coverage increased (+0.09%) to 91.881% when pulling 29f5a50071040acbd4d6adf9f3bf075edb9c4876 on advance-find into dc36be0d813a8330a57bc079660bfc38e2a42964 on development.

Coverage increased (+0.09%) to 91.881% when pulling c88dd4cad90ac1999056e4cdee997e95ce1700c6 on advance-find into dc36be0d813a8330a57bc079660bfc38e2a42964 on development.


Snky github action testing

Closes #
Changes proposed in this pull request:

...

Looks good but could you give me a brief explanation?

Yes, sure.  But it still uses SNYK_TOKEN from my account which need to be changed to a service account. I couldn't set up a service account as I didn't have admin access rights in Snyk.
The configuration was as below -

Copied the API_TOKEN from my account (which ideally need to be service account) in Snyk
Create  and add SNYK_TOKEN variable in stan-client project (Settings -> Secrets and variables -> Repository secretes) with about value

Then added github actions required

snyk/actions/node@master is a pre-created container available (https://docs.snyk.io/integrations/ci-cd-integrations/github-actions-integration/snyk-node-action)
I believe it runs , 'snyk test' by default
'command' - to give specific  snyk commands if need to be changed in our case we use 'code test' to avoid dependency scanning
args -
--severity-threshold=high    - will only look for high severity warnings
--sarif-file-output=snyk.sarif  - to create an output file for results

continue-on-error: true - To avoid blocking CI run in case  snyk test fails
The following part is to upload Snyk scan results to GitHub Code Scanning
- name: Upload result to GitHub Code Scanning
uses: github/codeql-action/upload-sarif@v2
with:
sarif_file: snyk.sarif
But this is not yet working. I assume it only works for 'monitor' command which I am trying to check now.
Hope this makes sense.


path/filepath: Clean definition is not idempotent

Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (go version)?
version 1.9
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (go env)?
windows/amd64
Issue
The algorithm defined for the Clean function is faulty and, in addition, is not faithfully implemented.
In particular, the definition is faulty because the idempotent condition
Clean(Clean(x)) == Clean(x)

could fail, if Separator is not '/', as on Windows systems.
In fact, by the algorithm documentation, on Windows systems '/' would not be processed as Separator, until the final translation to Separator. Thus, applying Clean once or twice (with '/' translated to Separator at the end of the first cleaning step) could return different results, contrary to the idempotent condition.
In addition, the algorithm implementation slightly deviates from the documentation. In fact, every character that satisfies
os.IsPathSeparator(c) == true

is processed as Separator, not only Separator, as stated in the algorithm definition. On Windows systems, this is true for both: '\\' and '/'.
Please note that, if os.IsPathSeparator('/') is true for all available implementations, '/' is always processed as Separator. Thus, idempotence can be magically preserved, despite the errors in the algorithm definition and implementation.
Proposal
I propose to clarify the algorithm documentation and adapt the implementation as follows.
Documentation

change the first sentence to

Clean returns the shortest path name equivalent to path by purely lexical processing. First, it replaces any occurrence of slash by Separator. Then, it applies the following rules iteratively until no further processing can be done:

replace slash with Separator in the sentence

The returned path ends in a slash ...

remove the sentence

Finally, any occurrences of slash are replaced by Separator.

Implementation

add the follwing statement at the first line
path = FromSlash(path)

change any expression like os.IsPathSeparator(x) to
x == Separator

change the last line to:
return out.string()

Could you please provide a small program that demonstrates the issue.
Thank you

On 24 Nov 2017, at 09:55, larhun<EMAIL_ADDRESS>wrote:
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (go version)?
version 1.9
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (go env)?
windows/amd64
Issue
The algorithm defined for the Clean function is faulty and, in addition, is not faithfully implemented.
In particular, the definition is faulty because the idempotent condition
Clean(Clean(x)) == Clean(x)
could fail, if Separator is not '/', as on Windows systems.
In fact, by the algorithm documentation, on Windows systems '/' would not be processed as Separator, until the final translation to Separator. Thus, applying Clean once or twice (with '/' translated to Separator at the end of the first cleaning step) could return different results, contrary to the idempotent condition.
In addition, the algorithm implementation slightly deviates from the documentation. In fact, every character that satisfies
os.IsPathSeparator(c) == true
is processed as Separator, not only Separator, as stated in the algorithm definition. On Windows systems, this is true for both: '\' and '/'.
Please note that, if os.IsPathSeparator('/') is true for all available implementations, '/' is always processed as Separator. Thus, idempotence can be magically preserved, despite the errors in the algorithm definition and implementation.
Proposal
I propose to clarify the algorithm documentation and adapt the implementation as follows.
Documentation
change the first sentence to
Clean returns the shortest path name equivalent to path by purely lexical processing. First, it replaces any occurrence of slash by Separator. Then, it applies the following rules iteratively until no further processing can be done:
replace slash with Separator in the sentence
The returned path ends in a slash ...
remove the sentence
Finally, any occurrences of slash are replaced by Separator.
Implementation
add the follwing statement at the first line
path = FromSlash(path)
change any expression like os.IsPathSeparator(x) to
x == Separator
change the last line to:
return out.string()
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

The issue can be demonstrated by a program only if os.IsPathSeparator('/') returns false, that never occurs on my Windows system.
Therefore, to prove that the algorithm definition is faulty, a modified path/filepath package is provided: github.com/larhun/filepath is a copy of the original, except that any occurrence of os.IsPathSeparator in the Clean function is replaced by IsPathSeparator, defined as
func IsPathSeparator(s byte) bool {
    return s == Separator
}

that is true only for Separator, conforming to the algorithm defined for the Clean function.
Then, the following program prints false.
package main

import(
    "fmt"
    "github.com/larhun/filepath"
)

func main(){
    p1:="/../../.."
    p2:=filepath.Clean(p1) // returns "\..\..\.."
    p3:=filepath.Clean(p2) // returns "\"
    fmt.Println(p2==p3)    // i.e. Clean(Clean(p1)) != Clean(p1)
}

For an idempotent function, the expected value is true.

Do I understand correctly that you're saying the issue occurs only when a condition is met - but that condition cannot happen?

No, I am not saying that. The issue is about documentation and implementation, that are faulty and not consistent.
The documentation states:

Clean returns the shortest path name equivalent to path by purely lexical processing. It applies the following rules iteratively until no further processing can be done:
1. Replace multiple Separator elements with a single one.
2. Eliminate each . path name element (the current directory).
3. Eliminate each inner .. path name element (the parent directory)
   along with the non-.. element that precedes it.
4. Eliminate .. elements that begin a rooted path:
   that is, replace "/.." by "/" at the beginning of a path,
   assuming Separator is '/'.

The returned path ends in a slash only if it represents a root directory, such as "/" on Unix or C:\ on Windows.
Finally, any occurrences of slash are replaced by Separator.
If the result of this process is an empty string, Clean returns the string ".".

I am saying that the previous algorithm is not right, because it states that only Separator is processed, before finally replacing any slash occurrence. If this was implemented precisely, then the Clean function could not be idempotent on Windows systems, as shown by the provided program.
However, the implementation deviates from the documentation and processes as  Separator any character that is true for os.IsPathSeparator and, on Windows systems, that includes the slash. Then, magically, the Clean function results idempotent, despite the wrong algorithm and the wrong implementation.
The documentation should not be misleading. The right algorithm should start by first replacing the slash with Separator. The right implementation should perform as stated by the algorithm.

If the implementation is faulty as claimed, there must be a way how to demonstrate the issue with that faulty implementation. But earlier you wrote:

The issue can be demonstrated by a program only if os.IsPathSeparator('/') returns false, that never occurs on my Windows system.

Is there any other system where os.IsPathSeparator('/') returns false? Because if there isn't any such then somewhere we have contradicting pieces of information.

To clarify my previous notes, there are two errors that translate into a correct behavior.

The first mistake is the definition of the algorithm, which formally does not guarantee the expected behavior, as shown by the provided program.

The second is the implementation, which differs from the stated algorithm, but eventually achieves the expected result.

Therefore,

The definition should be changed for the clarity and quality of the algorithm documentation.

The implementation is not true, but it does work as long as os.IsPathSeparator does not have a new system implementation that returns false for the slash. It should be changed for the quality of the source code.

I propose to change both.

Any changes to this package will affect millions of users that import it. The correct behavior is the one that causes the least surprise and adheres to the compatibility guidelines.
The risk involved in changing the algorithm is that the new algorithm will be incorrect (and that this incorrectness will also be inconsistent). File path handling is subtle, and requires many special cases to get right. That is probably the reason the code looks the way it does now.
On a system where '/' isn't a separator, what hope is there for correctness anyway?

I don't see any reason to change the implementation.
If you think that we should change the documentation, by all means send a pull request.  Thanks.


Incorrect onTapHint being applied

b/161765861
This seems like a regression since onTapHint seemed to work correctly before. Both cases produces different results, but the issue should be related.
Case 1
Semantics(
  button: true,
  onTap: () {},
  onTapHint: 'onTapHint',
  child: Container(color: Colors.blue, width: 100, height: 100, child: Text('text')),
)

Expected: TalkBack should read "text, Button" / "Double-tap to onTapHint."
Observed: TalkBack reads "text, Button" / "Double-tap to text."
Case 2
Semantics(
  button: true,
  onTap: () {},
  onTapHint: 'onTapHint',
  child: Container(color: Colors.blue, width: 100, height: 100),
)

Expected:
TalkBack should read "Button" / "Double-tap to onTapHint."
Observed: App crashes with the following stack trace:
[ERROR:flutter/shell/platform/android/platform_view_android_jni_impl.cc(43)]
java.lang.ArrayIndexOutOfBoundsException: length=0; index=0
[   +7 ms] E/flutter (17057): 	at io.flutter.view.AccessibilityBridge.updateCustomAccessibilityActions(AccessibilityBridge.java:1264)
[   +4 ms] E/flutter (17057): 	at io.flutter.view.AccessibilityBridge$1.updateCustomAccessibilityActions(AccessibilityBridge.java:260)
[   +4 ms] E/flutter (17057): 	at io.flutter.embedding.engine.FlutterJNI.updateCustomAccessibilityActions(FlutterJNI.java:500)
[   +4 ms] E/flutter (17057): 	at android.os.MessageQueue.nativePollOnce(NativeMethod)
[   +3 ms] E/flutter (17057): 	at android.os.MessageQueue.next(MessageQueue.java:336)
[   +4 ms] E/flutter (17057): 	at android.os.Looper.loop(Looper.java:174)
[   +4 ms] E/flutter (17057): 	at android.app.ActivityThread.main(ActivityThread.java:7356)
[   +4 ms] E/flutter (17057): 	at java.lang.reflect.Method.invoke(Native Method)
[   +3 ms] E/flutter (17057): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
[   +4 ms] E/flutter (17057): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
[   +4 ms] E/flutter (17057):
[   +4 ms] F/flutter (17057):
[FATAL:flutter/shell/platform/android/platform_view_android_jni_impl.cc(1019)]
Check failed: CheckException(env).

Unfortunately, this was a result of a g3 roll that contained 4 massive engine rolls.  FYI @goderbauer @justinmc

Looks related to https://github.com/flutter/flutter/issues/60926, the crash is the same.

@chunhtai is working on a fix for the crash: https://github.com/flutter/engine/pull/19623

Case 2

logs
Launching lib/main.dart on SM M305F in debug mode...
✓ Built build/app/outputs/flutter-apk/app-debug.apk.
Connecting to VM Service at ws://<IP_ADDRESS>:50358/B_YM0aSMW0U=/ws
E/flutter ( 3862): [ERROR:flutter/shell/platform/android/platform_view_android_jni_impl.cc(43)] java.lang.ArrayIndexOutOfBoundsException: length=0; index=0
E/flutter ( 3862): 	at io.flutter.view.AccessibilityBridge.updateCustomAccessibilityActions(AccessibilityBridge.java:1264)
E/flutter ( 3862): 	at io.flutter.view.AccessibilityBridge$1.updateCustomAccessibilityActions(AccessibilityBridge.java:260)
E/flutter ( 3862): 	at io.flutter.embedding.engine.FlutterJNI.updateCustomAccessibilityActions(FlutterJNI.java:500)
E/flutter ( 3862): 	at android.os.MessageQueue.nativePollOnce(Native Method)
E/flutter ( 3862): 	at android.os.MessageQueue.next(MessageQueue.java:336)
E/flutter ( 3862): 	at android.os.Looper.loop(Looper.java:197)
E/flutter ( 3862): 	at android.app.ActivityThread.main(ActivityThread.java:7807)
E/flutter ( 3862): 	at java.lang.reflect.Method.invoke(Native Method)
E/flutter ( 3862): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
E/flutter ( 3862): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1047)
E/flutter ( 3862):
F/flutter ( 3862): [FATAL:flutter/shell/platform/android/platform_view_android_jni_impl.cc(1019)] Check failed: CheckException(env).
F/libc    ( 3862): Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 3862 (.master_flutter), pid 3862 (.master_flutter)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build fingerprint: 'samsung/m30ltedd/m30lte:10/QP1A.190711.020/M305FDDU5CTF2:user/release-keys'
Revision: '4'
ABI: 'arm64'
Timestamp: 2020-07-23 18:59:57+0530
pid: 3862, tid: 3862, name: .master_flutter  >>> com.example.master_flutter <<<
uid: 10335
signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
Abort message: '[FATAL:flutter/shell/platform/android/platform_view_android_jni_impl.cc(1019)] Check failed: CheckException(env).
'
    x0<PHONE_NUMBER>000000  x1<PHONE_NUMBER>000f16  x2<PHONE_NUMBER>000006  x3  0000007fdc622eb0
    x4<PHONE_NUMBER>000080  x5<PHONE_NUMBER>000080  x6<PHONE_NUMBER>000080  x7<PHONE_NUMBER>000000
    x8<PHONE_NUMBER>0000f0  x9  b4c13c24148709a9  x10<PHONE_NUMBER>000001  x11<PHONE_NUMBER>000000
    x12 fffffff0fffffbdf  x13<PHONE_NUMBER>a1baf8  x14<PHONE_NUMBER>000000  x15 00005cc70d39f6ae
    x16<PHONE_NUMBER>0d78c0  x17<PHONE_NUMBER>0b52d0  x18<PHONE_NUMBER>3c4000  x19<PHONE_NUMBER>000f16
    x20<PHONE_NUMBER>000f16  x21 00000000ffffffff  x22 00000072a07755c8  x23 00000000ffffffff
    x24<PHONE_NUMBER>000018  x25<PHONE_NUMBER>000018  x26<PHONE_NUMBER>0000d6  x27<PHONE_NUMBER>0000bc
    x28<PHONE_NUMBER>000031  x29 0000007fdc622f50
    sp  0000007fdc622e90  lr<PHONE_NUMBER>06a038  pc<PHONE_NUMBER>06a064
backtrace:
      #00 pc<PHONE_NUMBER>083064  /apex/com.android.runtime/lib64/bionic/libc.so (abort+160) (BuildId: f0e764f03a5004a006cd457ee8a428dc)
      #01 pc<PHONE_NUMBER>25bac0  /data/app/com.example.master_flutter-agc9s7uhZZcwSmqLLX3UCg==/lib/arm64/libflutter.so (BuildId: 4176bb07918ad946d275e1431bc426eaffd6ad96)
      #02 pc<PHONE_NUMBER>0018cc  [anon:.bss]
Lost connection to device.
Exited (sigterm)

flutter doctor -v
[✓] Flutter (Channel master, 1.21.0-2.0.pre.95, on Mac OS X 10.15.6 19G73, locale en-GB)
    • Flutter version 1.21.0-2.0.pre.95 at /Users/tahatesser/Code/flutter_master
    • Framework revision 50b87dc6c3 (19 hours ago), 2020-07-22 11:09:59 -0700
    • Engine revision dcc9a4048d
    • Dart version 2.9.0 (build 2.9.0-21.0.dev 9dca49e71e)

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.1)
    • Android SDK at /Users/tahatesser/Code/sdk
    • Platform android-30, build-tools 30.0.1
    • ANDROID_HOME = /Users/tahatesser/Code/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.6)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 11.6, Build version 11E708
    • CocoaPods version 1.9.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.0)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin version 47.1.2
    • Dart plugin version 193.7361
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

[✓] VS Code (version 1.47.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.12.2

[✓] Connected device (5 available)
    • SM M305F (mobile)      • 32003c30dc19668f          • android-arm64  • Android 10 (API 29)
    • Taha’s iPhone (mobile) • 00008020-001059882212002E • ios            • iOS 13.6
    • macOS (desktop)        • macos                     • darwin-x64     • Mac OS X 10.15.6 19G73
    • Web Server (web)       • web-server                • web-javascript • Flutter Tools
    • Chrome (web)           • chrome                    • web-javascript • Google Chrome 84.0.4147.89

• No issues found!

I confirm this is the same issue as https://github.com/flutter/flutter/issues/60926
applying the fix solved both crash and on tap hint announcement

close as dup


Sync list of keymaps to xrdp_keyboard.ini

Some new keymaps were added, but xrdp/xrdp_keyboard.ini was not updated with it.
So weren't the Makefiles, but I've got a patch for that already, which will be submitted as part of a larger amount of push requests upcoming soon. For now:
--- a/instfiles/Makefile.am
+++ b/instfiles/Makefile.am
@@ -14,10 +14,14 @@ startscript_DATA =
km-040c.ini
km-0410.ini
km-0411.ini \

km-0414.ini
km-0415.ini \
km-0416.ini
km-0419.ini
km-041d.ini
km-0807.ini \
km-080c.ini \
km-0813.ini
km-0816.ini
km-e0010411.ini
km-e0200411.ini \

fixed and merged, please tell me if looks good.
https://github.com/neutrinolabs/xrdp/pull/278
please reopen if its not good enough

Sorry, not enough: the xrdp_keyboard.ini must also be adjusted to list them. I don't know which entries are correct for that, though.


SW-53417: Remove makeStyles everywhere in Terraware web (follow-up)

A small follow-up PR to add the sx prop to the TextField component, which was overlooked in the previous PR.

Thank you both, I appreciate it! 🙌


Update ClientSetup.md

Unfortunately, this PR is not valid because this is a very low quality change that you're suggesting. I encourage you to look at some of the documentation issues that we have already created here
For now, I'm closing this as invalid. Please let me know if I can help you in any other contributions that you'd like to make to our repository.


Migrate SPDX-JSON relationships to SBOM model

This is a follow up to #607 and #623
Closes #556
Changes made:

Removes the creation of relationships in the spdx22json format encoder and moves file-ownership relationship creation back to the main Catalog function.
Adds a new sbom.AllCoordinates that returns a slice of all coordinates referenced throughout all SBOM artifacts and relationships. This is needed to guarantee inclusion of all referenced files as nodes in the SBOM.
The power-user JSON document data structure has been replaced with the syftjson format model and the poweruser.Presenter has been deleted (as well as the remainder of internal/presenters 🎉 🌮 )
Combines file classifications, metadata, and contents fields for a location into a single entry. For this reason the fileMetadata, fileClassifications, and fileContents power-user JSON fields have been removed and replaced with a single files section. This requires bumping the JSON schema version from 1 to 2 .
Adds the application configuration to the output of the packages output (for syftjson format). This now matches the same behavior as power-user.

Notes:

schema/json/schema-2.0.0.json is not intended for manual review (~1000 lines).
this PR will remain in draft until #623 is merged.

Questions:

a future PR could implement syft1json and syft2json formats for backwards compatibility, though this is not planned at this time. Should we do this before bumping to schema version 2?
What should we do in cases where a package manager claims ownership of a file that does not exist (relative to the source.FileResolver)? Should we log and continue (what is implemented now)? Or make note of this in the SBOM?

Merging #623 has caused a lot of merge conflicts and rebasing that is needed (note the base branch changed, so the diff is way off now). I'll adjust shortly.

ok, rebased and cleaned up! 🤞

@spiffcs I've addressed all of your comments --ready for re-review! 🎉 🌮


Changed first word to uppercase

I usually show it on a line on its own so it's more appropriate for it to be uppercase in the case of 1 minute.

Great, thanks!


redux-logic login flow example?

Hi@all,
is there an example of how to implement the login flow using redux-logic and JWT? I'm very much interested in how to implement the refresh token logic to be specific. Could this be something that should be done in the validate function?
Thanks for any help

Here's some thoughts and ideas around JWT. Short answer is yes you can use redux-logic for things like auto-refreshing tokens and intercepting actions that need a valid JWT and refreshing before sending the action to the fetch logic.
Actual JWT creation and verification would be done server-side to keep things secure (since you don't want to go passing around your secret). And while redux-logic can be used on the server, most of these types of responses would be RPC style (request/response) so they would typically be done with a REST call or similar rather than an event based redux style call.
The server code that verifies JWT can also check to see when it is expiring and perform a refresh providing an updated cookie or header with the new JWT. If you are using a cookie to pass your JWT around then the client doesn't need to do much when it is refreshed, the browser just stores the new cookie. If you are not using cookies then you will want to look at the responses to see if they contain an updated JWT and store it appropriately.
So on the client you are basically showing them a login page until they have authenticated.
Login could just be it's own page where you simply post to the server and then once authenticated redirected to a different page or you can handle this like any async request (similar to any of the many examples) and when you receive the data you dispatch to update that you are authenticated and store the JWT if not using cookies.
You could certainly add some redux-logic code that sets a timer and performs a refresh before it expires.
const scheduleJWTRefreshLogic = createLogic({
  type: SCHEDULE_JWT_REFRESH,
  cancelType: LOGOUT, // cancel on action LOGOUT
  process({ getState, action, cancelled$ }, dispatch, done) {
    const refreshDelay = action.payload;
    const timer = setTimeout(() => {
      axios.post(url, data)
        .then(resp => resp.data)
        .then(data => {
          const nextRefreshDelay = calcDelay(data.jwt);
          dispatch({ type: JWT_UPDATE, payload: data.jwt });
          dispatch({ type: SCHEDULE_JWT_REFRESH,
                     payload: nextRefreshDelay });
        })
        .catch(err => {
          console.error(err);
          dispatch({ type: ERROR, payload: err, error: true });
        })
        .then(() => done());  // done dispatching
    }, refreshDelay);

    cancelled$.subscribe(() => clearTimeout(timer));
  }
});

OR using Observables it could look like this
const scheduleJWTRefreshLogic = createLogic({
  type: SCHEDULE_JWT_REFRESH,
  cancelType: LOGOUT, // cancel on action LOGOUT

  processOptions: {
    failType: ERROR
  },

  process({ getState, action) {
    const refreshDelay = action.payload;
    return Observable.timer(refreshDelay) // return obs
      .mergeMap(() =>
        Observable.ajax({ url, responseType: 'json' })
         .map(ret => ret.response)
         .mergeMap(data => { // create a couple actions to dispatch
           const nextRefreshDelay = calcDelay(data.jwt);
           return Observable.of(
             { type: JWT_UPDATE, payload: data.jwt },
             { type: SCHEDULE_JWT_REFRESH,
                     payload: nextRefreshDelay }
           );
         })
       );
  }
});

As for your suggestion about checking JWT during validate, yes, you could do a check in the validate hook. It might look something like this.
This is an example of a centralized JWT validate logic separate from the fetching logic. You could also do the validates in each fetch logic itself.
// check the JWT token in the payload of FETCH_REPOS or FETCH_WIDGETS
// actions. The token could also just been stored in state.
// If expired or expiring soon, refresh before continuing
const checkJWTLogic = createLogic({
  type: [FETCH_REPOS, FETCH_WIDGETS], // action types or regexes
  validate({ getState, action }, allow, reject) {
    checkToken(action.jwt, getState())
      .then(jwt => {
        allow({
          ...action,
          jwt  // using the new or updated jwt token
        });
      })
      .catch(err => {
        console.error(err);
        reject({ type: AUTH_ERR, payload: err, error: true })
      });
  }
});

function checkToken(jwt, state) {
  const expires = calcExpires(JWT); // negative if already expired
  if (expires < 0) { // expired
    return api.getJWTToken(state.x); // returns a promise to new JWT
  } else if (expires < THRESHOLD) { // expires soon
    return api.refreshJWTToken(jwt); // return a promise to refreshed JWT
  }
  // otherwise not expiring soon nothing to do, still valid
  return Promise.resolve(jwt); // promise to existing JWT
}

Thanks Jeff.
The server side is not using cookie so I will have to check the expiration manually on the client. That was very useful!


Lint --help

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #75   +/-   ##
=====================================
  Coverage     100%   100%
=====================================
  Files           9      9
  Lines         870    870
=====================================
  Hits          870    870

Impacted Files
Coverage Δ

ldap2pg/config.py
100% <ø> (ø)
:arrow_up:

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 ab5ccda...7a03d3f. Read the comment docs.


Build RPMs in mock

By building RPMs inside a mock chroot, we build RPMs more like koji and
brew would. This ensures that we build RPMs with the correct
libraries without additional cruft from a customized system.
Also, install RPMs from the mock after they are built instead of
compiling them with Ansible.
Signed-off-by: Major Hayden<EMAIL_ADDRESS>

Note that at this moment, the base tests are run first, then the image tests. We can split these in the pipeline since the deployment part is faster now (since we don't compile anything), but I'm trying to keep this change simpler for now.

The only issue found was the intermittent FAILED/FINISHED bug that intermittently appears on Fedora 32. https://osbuildci.cloud.paas.psi.redhat.com/job/osbuild/job/osbuild-composer/job/PR-603/69/display/redirect

🛑 Hold off on merging this for now. I'd like to get some of this mocking logic into ansible-osbuild instead.


Pass spdm_context to HAL functions

From #2487.
Integrators may instantiate multiple endpoints using the libraries in libspdm/library. However many of the Integrator-implemented functions in https://github.com/DMTF/libspdm/tree/main/include/hal/library assume a single instance. For example when libspdm_generate_measurement_summary_hash is called the function cannot discern between the multiple instances. To alleviate this the spdm_context should be passed to these functions. LIBSPDM_DATA_APP_CONTEXT_DATA can probably be used to identify the context. If not then #1951 can be implemented.

Closed via pull request #2592.


chore: Upgrade libp2p to v0.53.0

if you want to do it at once, there is this timeout related complexity we added to header-ex that is resolved by the libp2p changes


Use a named tuple for parsed commits

This improves readability as we can use attributes such as bump and descriptions instead of confusing numeric indices.

Nice 👍