Add a "repeated flag" to parameter nodes

It's possible to repeat parameters in method definitions like so:
def foo(_a, _a)
end

The compiler needs to know to adjust the local table size to account for these duplicate names.  We'll use the repeated parameter flag to account for the extra stack space required

@kddnewton mind reviewing again? I think I addressed the feedback.

I wonder what is the right approach here.
@andrykonchin and I were thinking to open an issue or discussion about this case:
class P
  def m(*a) = p a
end

class C<P
  def m(_,_,_)
    _ = 4
    super
  end
end

C.new.m(1,2,3) # => [4, 2, 3]

So the 1st _ is really a different variable than the 2nd _ and 3rd _.
And assigning _ only assigns the first one.
Notably ruby --dump=insns -e 'def m(_,_); [_,_]; end' shows a local table of size 2 (and not 1).
But Prism currently only declares one local for that case.
The JRuby parser defines multiple variables in that case:
Source:
def m(_,_); [_,_]; end

AST:
RootParseNode
  DefnParseNode:m
    ArgsParseNode
      ArrayParseNode
        ArgumentParseNode:_
        ArgumentParseNode:_$0
      ArrayParseNode
      ArrayParseNode
      ArrayParseNode
    ArrayParseNode
      LocalVarParseNode:_
      LocalVarParseNode:_

So I'm thinking it would be nice if Prism would really consider these _ variables to be different because they are obviously separate semantically.
Then the Prism locals would also reflect how many variables there are.
Regarding naming, one trick is to use an invalid local variable name like _$0 in JRuby parser or _%0 or %_0.
Then they can easily be filtered and won't appear as accessible as Ruby local variables.
Starting with an invalid character seems best to ease filtering so e.g. the 2nd _ becomes %_2 and the 2nd _a becomes %_a2.
(binding.local_variable_get :"NAME" can be used to check whether it's a valid local var name)
I think the repeated flag in this PR is harder to process and error-prone, i.e., if anything uses the Prism AST and see two _ variables they will just think it's the same variable (incorrect semantically) vs if they are actually different variable names then there is no ambiguity and no need for every usage of Prism to actually expand duplicated variables into separate variables.
As a note, while the added flag on *ParameterNode does not increase memory for C structs (they always have flags), and only increase if the flag is set in serialized form, it does increase the memory for all Java classes for *ParameterNode, because those only have a short flags; field if there is any flag.

For comparison to the JRuby AST above, this is the current Prism AST for def m(_,_); [_,_]; end:
$ bin/parse -e 'def m(_,_); [_,_]; end'
...
        └── @ DefNode (location: (1,0)-(1,22))
            ...
            ├── parameters:
            │   @ ParametersNode (location: (1,6)-(1,9))
            │   ├── requireds: (length: 2)
            │   │   ├── @ RequiredParameterNode (location: (1,6)-(1,7))
            │   │   │   └── name: :_
            │   │   └── @ RequiredParameterNode (location: (1,8)-(1,9))
            │   │       └── name: :_
            │   ├...
            ├── body:
            │   @ StatementsNode (location: (1,12)-(1,17))
            │   └── body: (length: 1)
            │       └── @ ArrayNode (location: (1,12)-(1,17))
            │           ├── flags: ∅
            │           ├── elements: (length: 2)
            │           │   ├── @ LocalVariableReadNode (location: (1,13)-(1,14))
            │           │   │   ├── name: :_
            │           │   │   └── depth: 0
            │           │   └── @ LocalVariableReadNode (location: (1,15)-(1,16))
            │           │       ├── name: :_
            │           │       └── depth: 0
            │           ├...
            ├── locals: [:_]
            ├...

Looking at it it all seems too clear that there is a single _ variable, but there are 2 semantically.
The repeated flag addresses it for the RequiredParameterNodes, but not for LocalVariableReadNode, which is then from the AST not clear to which variable it refers.

It is funny because I was discussing this variable to someone this weekend and it appears '' as a user visible variable is the first '' encountered while parsing.  So:
def foo(,); p _; end; foo(1, 2); # "1"
def foo(_, (a, _); p _; end; foo(1, [2,3]) # "3"
For this second case either first _ depth-first parse use of _ is used OR first is bound and the destructuring assigns to first _ (I believe it is the second case but it is confusing) The first example is interesting because why is that 1 and not 2?  I don't personally think people should depend on being able to access this but Ruby is what it does and we seem to have this single variable name with weird value assignment behavior that also does not warn.  I have a theory on why this exists but I won't bore anyone.
That aside the semantics are only one _ is actually a valid variable for use.  The others are specifying we do not care about them as user-visible values (and honestly they should not have allowed _ to be accessed at all).  The fact that the legacy parser (in JRuby) assigns is just an implementation detail (in fact it is a vestigial artifact from AST interpreter days and has not been used in years).  In Prism this is how we do it:
    /* '_' can be seen as a variable only by its first assignment as a local variable.  For any additional
     * '_' we create temporary variables in the case the scope has a zsuper in it.  If so, then the zsuper
     * call will slurp those temps up as it's parameters so it can properly set up the call.
     */
    Variable argumentResult(RubySymbol name) {
        // |a,| case
        if (name == null) return temp();

        boolean isUnderscore = name.getBytes().realSize() == 1 && name.getBytes().charAt(0) == '_';

        if (isUnderscore && underscoreVariableSeen) {
            return temp();
        } else {
            if (isUnderscore) underscoreVariableSeen = true;
            return getNewLocalVariable(name, 0);
        }
    }

In this new parsing world we just assign a temp and do not pollute the variable table (obviously JRuby using register based design makes this simple).  The main point here though is there is no semantic reason why _ should have extra local variables.  Users cannot seem them and implementers (at least JRuby) are not using them to implement zsuper.   HOWEVER
Marking the real _ would have two benefits:

The above code would be simpler because I would not have to examine the name of ever parameter.  I would check type or field or flag.
If you were to write a Ruby script to evaluate variable uses it would be handy to know which _ was the user-visible one.  Having consumers of the tree figure this out is probably beyond most people's general knowledge.  As we can see above it was above mine and I implemented it a few times.

@kddnewton I think we should consider annotating the real _ for item 2 but it would not hurt for number 1 but obviously it is not a big burden to do what I did.
@eregon I don't see value in adding these extra _ as variables.  It feels like impl detail and not semantics.

def foo(_, (a, _); p _; end; foo(1, [2,3]) # "3"

Wow that is nasty.
I think it's worth reporting at https://bugs.ruby-lang.org/, at least to discuss it there if intended or bug.
BTW this PR labels the _ after a,  as repeated_parameter so that's inconsistent in Prism vs CRuby.

@eregon I don't see value in adding these extra _ as variables. It feels like impl detail and not semantics.

That's the part we disagree. They are semantics because they affect the behavior of Ruby and it is observable (an implementation detail OTOH would not be observable from Ruby code).
Any correct semantic usage of Prism will require to deal with this edge case, if such _ code is ever provided as input.
I think it's easier to deal with the edge case if Prism recognizes them as separate variables (notably it would not require state in the translator/compiler to deal with that, in general we avoid state as much as possible in TruffleRuby translator as it works poorly with lazy translation).
But in the end it's 2 different representations for the same thing. For the flag one still needs to manually number the various _ though.
I wish multiple _ would just be the same variable and writes to such parameters would just overwrite each other (or just multi _ be a SyntaxError), but it's not the semantics of Ruby 3.3 and before.

AFAIK matz says that assignments to repeated _ prefixed variables has undefined behavior. I need these flags just because the locals table that prism provides is actually a set so we can't know the right stack size on method entry.
For example:
def foo(a, _a, _a); end
foo(0, 1, 2)

The caller will push 3 items on the stack, but if we only look at the local table provided by prism, then we'll assume the stack size is only 2.  This flag just helps CRuby do stack size book keeping, it's not a statement about actual behavior in the language.

@eregon wrote:

I wish multiple _ would just be the same variable and writes to such parameters would just overwrite each other (or just multi _ be a SyntaxError), but it's not the semantics of Ruby 3.3 and before.

I think some people use variable names that start with '_' to indicate that these are ignored. That would suggest that such variables are not accessible at all. This would solve the problem with duplication automatically.

I need these flags just because the locals table that prism provides is actually a set so we can't know the right stack size on method entry.

Right, it's very similar to the issue enebo and I are mentioning.
undefined behavior does not exist in Ruby, it's what CRuby does or doesn't.

the locals table that prism provides is actually a set

I think that's the part we could fix, we could have Prism list all the semantically-different variables in its local tables (by renaming duplicates as suggested above).
Then it would be a "set/table of all semantics local variables in that scope" vs currently it's a "set of names in the source, not necessarily corresponding to one variable but in some cases to multiple variables".
I think no one expect a "locals table" to not include all semantic/required local variables, otherwise it's incomplete and cannot be relied upon, isn't it?
I can see the renaming is not ideal (introducing variables with a different name than in the source) but I think it's the cleanest solution here.
It is possible to workaround this with the new flag, but it's less convenient and I think every semantic usage of Prism will have to manually workaround this.


BeforeEach of login sometimes fails in CI

Describe the bug
BeforeEach of login sometimes fails in CI.
Steps to reproduce
It's flaky. Sometimes it passes, while sometimes it fails.
Expected behavior
It should always pass.
Screenshots or logs
https://github.com/runfinch/finch/actions/runs/3537401635/jobs/5937342920:
  time="2022-11-24T03:26:51Z" level=fatal msg="failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error running hook #0: error running hook: exit status 1, stdout: , stderr: time=\"2022-11-24T03:26:51Z\" level=fatal msg=\"listen tcp <IP_ADDRESS>:52086: bind: address already in use\"\nFailed to write to log, write /home/ec2-user.linux/.local/share/nerdctl/1935db59/containers/finch/cf16f87c4026e285e41a856a4364e8ca81c312fd7e5da5fe7d0ec1179e98f6c0/oci-hook.createRuntime.log: file already closed: unknown"
  time="2022-11-24T03:26:51Z" level=fatal msg="exit status 1"
  [FAILED] in [BeforeEach] - /Users/ec2-user/go/pkg/mod/github.com/runfinch/common-tests@v0.1.0/command/command.go:122 @ 11/24/22 03:26:51.812

Closing as it went away for a long time. Feel free to reopen if it happens again.


Please include HENDRICS and srt-single-dish-tools

Thanks. I've added another commit to remove them from the python2 list.


Migrate repository to Python

Describe the issue
For some reason the JS2Py bridge seems to go unstable and hang every so often, but doesn't show any outward sign if what's the underlying problem... As a testing idea, I'm going to see how doable it would be to convert the translations repo to pure Python. It has so few js dependencies that I really think it ought to be doable (wit ChatGPT of course), and that would be a cleaner solution.
Which language do you see the issue with?
English
Are you willing to submit a pull request to fix the issue?
No
Other details
No response
Acknowlegdements

[X] I have searched this repository to see if the issue has already been reported.
[X] I have filled in the template to the best of my ability.
[X] I have written an informative title.

@alexander0042 Just curious what the status of this is. Its been a few days and I haven't seen any updates or a PR to this repo with the python code. Still occasionally see downtime with the dev endpoint but it hasn't been as bad as it was over the weekend.

@alexander0042 I was able to get the tests working and I put everything together into its own branch here: https://github.com/Pirate-Weather/translations/tree/python.
The current language tests are pretty easy to move the nodejs tests into python as the format is pretty similar. I would have added them but the tests would fail as the languages haven't been added yet.


Release Zhumir

Codecov Report

Merging #590 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   96.43%   96.45%   +0.01%
==========================================
  Files         123      123
  Lines        3707     3724      +17
  Branches      326      325       -1
==========================================
+ Hits         3575     3592      +17
  Misses         94       94
  Partials       38       38

Impacted Files
Coverage Δ

datahub/omis/order/urls.py
100% <ø> (ø)
:arrow_up:

datahub/omis/order/views.py
100% <100%> (ø)
:arrow_up:

datahub/omis/order/serializers.py
100% <100%> (ø)
:arrow_up:

datahub/omis/order/validators.py
97.88% <100%> (+0.11%)
:arrow_up:

datahub/omis/payment/constants.py
100% <100%> (ø)
:arrow_up:

datahub/search/event/serializers.py
96.55% <0%> (-0.06%)
:arrow_down:

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 ace1cd7...e6e726c. Read the comment docs.


templates-no-negated-async not reporting some failures

Reproduction:
{
  "rules": {
    "templates-no-negated-async": true
  }
}

@Component({
  selector: 'app-test',
  template: `

    {{ (foo | async) == false }} // not reporting failure here
    {{ !(bar | async) }} // not reporting failure here
  `,
})
export class TestComponent {}

@Component({
  selector: 'app-test',
  template: `

 // idk if it's intentional, but it's not reporting failure
    {{ (foo | async) == false }} // not reporting failure here
    {{ !(bar | async) }} // not reporting failure here
    {{ !(isHandset | async) ? 'dialog' : 'navigation' }} // not reporting failure here
  `,
})
export class TestComponent {}

@wKoza, can you apply the label bug here?

I've started to looking at this issue and I found what's the problem:
https://github.com/mgechev/codelyzer/blob/5a84041253dc07ffd6445bef5d6d6733ff43fecd/src/templatesNoNegatedAsyncRule.ts#L28
For the first template above the results are:
endLeftSpan = 28
startRightSpan = 33
this.codeWithMap.code.slice(endLeftSpan, startRightSpan); returns "t a o" (from *ngFor line).
It seems that there is a real problem with multiline templates. It's only getting the code from the first line, I really don't know why.

Question:
Why do we always assume that the expression is being compared to false? What if the user put ! trying to represent a nil check (null/undefined)?
@Component({
  selector: 'app-test',
  template: '
Some content
'
})
class TestComponent implements OnInit {
  data$: Observable<User | undefined>;

  ngOnInit() {
    this.data$ = someServiceThatCanReturnUserOrUndefined;
  }
}

In this case, an user is trying to use ! to check if Observable emits a undefined or even a null value, so the expression could be changed to:

Some content

Some content

... instead, we replace the expression with === false, what brings an unexpected behavior, a completely unsafe replace.

Also, there are real failures related to the current replacement:

Something

... After running --fix:

Something

IMHO, we should remove the "fix" option for this rule, since we can't assume what the "expression" is trying to do... or maybe leave only the replace only for strict equality (== false -> === false).
@wKoza @mgechev let me know your thoughts.

bump...

IMHO, we should remove the "fix" option for this rule, since we can't assume what the "expression" is trying to do... or maybe leave only the replace for strict equality (== false -> === false).

Yes, we should remove the fix since it's not safe.

@mgechev I'm still trying to find a valid use case for this rule, do you have one?
Here...
https://github.com/mgechev/codelyzer/blob/5a84041253dc07ffd6445bef5d6d6733ff43fecd/src/templatesNoNegatedAsyncRule.ts#L50
... we say "Async pipes can not be negated". Why not?.
Take a look at this sample:
@Component({
  selector: 'app-root',
  template: `

      Loading...

  `
})
export class AppComponent  {
  obs$ = observableOf([1, 2, 3]).pipe(
    delay(2000) // simulates an http request
  );
}

What would be the benefits of an explicit check like this <div *ngIf="(obs$ | async) === null" instead of what I did above?
Source sample:

@mgechev just to complement: unless there's a valid case that this rule could be useful, I'm voting in favor of deprecate this rule.

I'll look at the story behind it over the weekend.

@mgechev Friendly bump, have you looked?

Yes, here's the motivation.

@mgechev better close this and create a new issue about deprecate this rule or keep the discussion here?

Sounds good!

@rafaelss95 since we had this conversation, we reevaluated the pros and cons of this rule and introduced it in Google. Let's keep it as part of codelyzer.

@mgechev Okay, can we just update the following failure message?https://github.com/mgechev/codelyzer/blob/bc5a7ec5fd94e665423c77cb4c08fa98d2e2dfe9/src/templatesNoNegatedAsyncRule.ts#L68

Sure. How do you suggest to update it?


Simplify Elasticsearch environment variables

This commit tries to simplify how we configure Elasticsearch via environment variables, for virtualenv/local Docker/deployed Docker.
Currently, as of #6695, Django will use the combination of ES7_HOST and ES_PORT to configure the Elasticsearch client. ES_HOST is no longer used anywhere, since our migration to Elasticsearch 7.x. In Django settings, these default to localhost/9200 if the environment variables are not defined.
With this change, the following configurations are still supported:

Local virtualenv - these settings are now no longer set in .env, but default to localhost/9200 for a locally-running Elasticsearch. The definitions have been left in .env_SAMPLE commented out for documentation purposes.

Local Docker - port defaults to 9200, but ES7_HOST is set in docker-compose.yml to "elasticsearch" instead.

Deployed Docker stack - port defaults to 9200, but ES7_HOST is set in docker-stack.yml to "elasticsearch" instead.

Notes and todos
If/when this is merged, there is some cleanup to be done in our internal Ansible deployment scripts to remove reference to the now-deprecated ES_HOST environment variable.
Checklist

[x] PR has an informative and human-readable title

PR titles are used to generate the change log in releases; good ones make that easier to scan.
Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".

[x] Changes are limited to a single goal (no scope creep)
[x] Code follows the standards laid out in the CFPB development guidelines
[x] Future todos are captured in comments and/or tickets

@higs4281 thanks, nice catch.

ccdb-data-pipeline, which I can adjust.

Technically this doesn't have to be consistent with this repo, does it? Although...

ccdb5-api, which will default properly

Hm, but this won't default properly if using the Docker setup, after my changes, if we stop setting ES_HOST.

It might have made sense to use ES_HOST everywhere now that 7 is our only option, but maybe it's simplest to stick with ES7 than track them all down.

That was my thinking as well, but maybe the better thing to do is instead deprecate our use of ES7_HOST in cf.gov and instead use ES_HOST. This will avoid having to modify CCDB code, although it would require some Ansible changes. What do you think of that approach vs. this one?

@chosak
The CCDB changes to use ES7_HOST are trivial and only affect local operations, which are somewhat rarely used. This doesn't affect anything in prod ccdb indexing, and prod ccdb searching already uses ES7_HOST.
Given how fond of breaking changes ES seems to be, I don't think it's a big sin to have a version-specific var. The next time we upgrade, we'll probably need two settings again to get Django and CCDB converted in tandem, and having the old tagged to ES7 might help with clarity.
I'd vote to proceed with ES7_HOST.

If we want to go with the pristine ES_HOST, we'd probably need this sequence:

Make sure ES_HOST is set in all environments
Change the CCDB repos to use ES_HOST only.
After those changes clear, remove ES7_HOST from cfgov code, docker configs, jenkins-automation jobs and ansible.

I ran into another issue with the prod-like Docker image: the suggested command doesn't load docker-compose.override.yml and thus doesn't read in the .env file (normally done this way), which means that the only environment variables passed to the Apache container are the ones explicitly defined here. So, the container doesn't get a value for ES7_HOST right now, which means that any Elasticsearch interactions fail.
Currently we define ES_HOST in docker-compose.yml, but rely on the .env behavior to get ES7_HOST in there, inconsistently, and unnecessarily, it seems to me.

If we want to go with the pristine ES_HOST
Make sure ES_HOST is set in all environments

It looks to me from inspecting the Ansible code that we're already passing the same URL for both ES_HOST and ES7_HOST, so I think we're good there.

Change the CCDB repos to use ES_HOST only.

I guess that's this line? Is there any other place or documentation that would need to be updated if we made this change?

I guess that's this line? Is there any other place or documentation that would need to be updated if we made this change?

"this line" is fixed in ccdb5-api
That version of ccdb5-api is added to requirements in PR 6753


Need Parrot Security added to accepted distros PLEASE...

Feature Description
You need to please add support for Parrot Security OS, this distro is quickly becoming as popular as Kali. Many of us prefer it over Kali, me included! This would only make your app even more popular.
Alternatives Solutions
n/a
Example Use Case
n/a

Thanks for the suggestion but no, we won't be officially supporting any additional Linux distributions any time soon.
There's a few reasons for this. First there's nothing stopping you from running King Phisher on Parrot, you may run into a few minor install issues but that's it. That does however bring me to my second point, the distros we've listed as supported are periodically tested, monitored for releases and documented. Adding an additional distribution is a lot of in going work for us and is not currently aligned with an initiative that we are pursuing.
Again thanks for the feature suggestion hopefully some time in the future we'll be in a better position to support additional distributions.


[Accessibility][Keyboard - Property Pane_Ask a question]: Focus indicator is not clear on all the sides over "Always prompt"check box.

User Experience
Keyboard users will be affected if focus indication is not clearly visible on all the 4 sides of over "Always prompt"check box. Users will face difficulty if there is no clear focus indicator over an interactive control in the application.
Test Environment
Browser: Microsoft Edge Canary Version: 82.0.425.0 (Official build)
OS build: 2004 (19564.1005)
Screen Reader: Narrator
URL: Bot Framework Composer
Prerequisite Here
Repro Steps:

Open the above mentioned URL in EDGE.
Navigate to already created dialog tree item and activate it. Such as- Greetings, Actions, EndDialog etc.
Click on "+" and select 'Ask a question'. Then select any of the available options except Oauth login.
Navigate to property pane appeared at right side of the screen and navigate to "Always prompt" checkbox using Tab key from keyboard.
Observer the issue.

Actual:
Focus indicator is not clear on all the sides over "Always prompt"check box. Keyboard focus is visible on the 3 sides of the checkbox.
Expected:
Focus indicator should be clearly visible on all the 4 sides over "Always prompt"check box.
Note:
Issue is observed for all the checkboxes in the Property Pane.
MAS Impacted: MAS2.4.7
Attachment:
MAS2.4.7_Checkbox.zip

Note to dev: Based off Fabric, it looks like focus is missing from surrounding both the checkbox and text:

Check out: https://developer.microsoft.com/en-us/fabric#/controls/web/checkbox

In the latest version, this is a text field and not a checkbox; this issue doesn't appear to be valid any more.

@ashish315 @awalia13, could we get confirmation that this issue can be closed?

As checked the UI has been updated. In the latest version, this is a text field and not a checkbox; this issue doesn't appear to be valid any more. So closing the bug.


test(olm): Add mutation tests for pre_key

This fixes:
MISSED   src/olm/messages/pre_key.rs:81:9: replace PreKeyMessage::session_id -> String with String::new() in 0.9s build + 3.2s test
MISSED   src/olm/messages/pre_key.rs:81:9: replace PreKeyMessage::session_id -> String with "xyzzy".into() in 0.8s build + 2.8s test

Relates to: #78

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

Project coverage is 85.03%. Comparing base (993410b) to head (aa23c91).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   84.92%   85.03%   +0.11%
==========================================
  Files          32       32
  Lines        1758     1758
==========================================
+ Hits         1493     1495       +2
+ Misses        265      263       -2

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


V1.3 UserGuide Changes

Added intro to features portion of UG
Removed almost all occurrences of "Address Book" within UG.

Left with json file which is named addressbook.json, should
we change this?
Minor fixes:
classcode prefix changed from c to cc
Multiple activities in add function should use multiple act
prefixes

Codecov Report

Merging #107 (ac35d4d) into master (1b08cb4) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #107   +/-   ##
=========================================
  Coverage     74.06%   74.06%
  Complexity      484      484
=========================================
  Files            81       81
  Lines          1442     1442
  Branches        166      166
=========================================
  Hits           1068     1068
  Misses          328      328
  Partials         46       46

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 1b08cb4...ac35d4d. Read the comment docs.


docs: improve

Summary

Test plan

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

Project coverage is 100.00%. Comparing base (a666086) to head (a5fa9a6).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #68   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files           14        14
  Lines          165       165
  Branches        39        39
=========================================
  Hits           165       165

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


Add functionality to specify "YAKS_TEST_ID"

Hi all!
This is a follow up on the issue 268 https://github.com/citrusframework/yaks/issues/268.
I added two checks at the beginning of the test to determine, if the istio proxy is ready. Here is the code:
Given Kubernetes resource polling configuration
| maxAttempts          | 10   |
| delayBetweenAttempts | 1000 |
Given Kubernetes pod ${YAKS_TEST_NAME}${YAKS_TEST_ID} should print Envoy proxy is ready
Given URL: http://localhost:15021
Then wait for path /healthz/ready
Unfortunately I still get the error, that the maven dependencies for the presto-driver could not be resolved. So this exception gets triggered before even one of the steps gets executed.
To check whether the istio proxy is ready I probably have to move that check into a pre script. Here it would be helpful to define the environment variable "YAKS_TEST_ID" so I can use the name of the test-pod in the pre script. Starting the test with "yaks test -e YAKS_TEST_ID=$RANDOM_ID file.feature" only added an additional environmental variable to the test pod and did not change the name of it.

Small update! I checked If I could access and wait for the test-pod in a pre-script. This is not possible. I seems like yaks is pulling the dependencies between the execution of the pre-script and before the execution of the first step. Currently we have istio disabled for the yaks tests, but it would be nice if one could add functionality to make this work with istio again, so our test represent a more realistic scenario.

Hi Nils, where do you get those Maven dependency errors? Is it in the YAKS test pod when the test is executed? Maybe you just need to add the dependency to yaks-config.yaml


Refactor and reuse code

Description:
This is "just" a refactor. As I was working on #98, I realize it would be simpler if there was a bit more potential for reusing code around. I am not sure that all of those change are really idiomatic go and feel like doing a PR that "just" move code around and remove duplicated step will make the next PR easier to move forward.
Checklist:

[x] Tests included.
[x] Lint and formatter run with no errors.
[x] Tests all pass.

Where applicable:
There should be no functional change. I have only the ability to test with podman at the moment and will be glad if anyone tried with docker.

I had a cursory look at PR and I would say that the refactor make sense to me but some thoughts come in mind... :-)
Looking at the amount of code changed I was thinking if it could be the right time to move fyne-cross as command of the fyne CLI app, something like fyne cross.
Having it as part of the fyne CLI will bring the following pros:

remove the direct dependency from the Docker image. This will reduce the number of releases and possible issues due to versions not aligned (see #85)
all the fyne CLI APIs can be invoked directly, no need to use the exec.Cmd (thinking about the build and package steps here)
uniform the CLI APIs and flags (see #46)
the support to FyneApp.toml and build metadata would not require code duplication (see https://github.com/fyne-io/fyne-cross/pull/87#issuecomment-1019441410)

Not strictly related but something could be helpful is to move the Docker image code into a dedicated repository (i.e. fyne-cross-image). Right now it they are different projects with a proper life-cycle that shares the same repo.
Thoughts?

I personally like the idea of moving fyne-cross to become fyne cross and get synchronized release. It will have to be discussed with the rest of the community. Maybe we should create a set of issues toward that goal in fyne-cross and aim for inclusion of fyne-cross in fyne for 2.3.

could be the right time to move fyne-cross as command of the fyne CLI app, something like fyne cross.

This is a great thing to work towards, however I don't think I would have it as a separate command (fyne cross) but more likely an extension of the whole command where we improve the cross-compile system and allow the fyne-cross path to be run if/when appropriate.

Good point, that makes sense to me. We already do a similar thing when building from a macOS with a darwin target

@Bluebugs it looks like the android command is not available:
The commands are:

	darwin-image  Build the docker image for darwin
	darwin        Build and package a fyne application for the darwin OS
	linux         Build and package a fyne application for the linux OS
	windows       Build and package a fyne application for the windows OS

	ios           Build and package a fyne application for the iOS OS
	freebsd       Build and package a fyne application for the freebsd OS
	version       Print the fyne-cross version information

Use "fyne-cross <command> -help" for more information about a command.```

@Bluebugs it looks like the android command is not available:

Indeed, good catch. I missed that part. Will fix it shortly.

I have added some thoughts around naming. However I was not able to add suggestions in most places as it is not clear of the intent from model presented. Can you please summarise what each of the new interfaces/structs are supposed to describe in one comment so we can understand the bigger picture @Bluebugs?

Sounds like a lot of good suggestion. I can have a pass at it tomorrow and we can iterate on the change after that.

Thanks @andydotxyz for the review. @lucor if you do have some time to look at this PR over the weekend, would really appreciate!

Looks like the new code requires Go 1.16 but the project still supports 1.15 so the builds are failing.

Looks like the new code requires Go 1.16 but the project still supports 1.15 so the builds are failing.

It was actually the tool we use for the github action that need 1.18, not the code introduced in this PR. All green now.

This is getting close but I am still lost in the terminology. Relationship between platform and cross builders is strange (android is both) and also image and engine are not in the dependency order I expected.
Is it possible to have a simple diagram or text overview of how these types are expected to fit together?

Small attempt at a diagram, not sure it is super usable:
classDiagram
   Command<|--Linux
   Command<|--Windows
   CrossBuilder<|--Linux
   CrossBuilder<|--Windows

   class Command{
     <<Interface>>
     +Run() error
   }
   class CrossBuilder{
     <<Interface>>
     +Build(ContainerImage)
     -setupContainerImage(flags, args) error
   }

   Command-->ContainerImage:Run() Use
   CrossBuilder-->ContainerImage:Build() Use
   CrossBuilder-->ContainerEngine:setupContainerImage() Use

   ContainerEngine<|--LocalContainerEngine
   ContainerImage<|--LocalContainerImage
   ContainerEngine<|--KubernetesContainerEngine
   ContainerImage<|--KubernetesContainerImage

   class ContainerEngine{
     <<Interface>>
     +createContainerImage(arch Architecture, OS string, image string) containerImage
   }

   class ContainerImage{
     <<Interface>>
     +Run(vol volume.Volume, opts options, cmdArgs []string) error
   }

That is really helpful thanks - thought it is missing the PlatformBuilder which was part of my confusion from earlier.
Apart from that the diagram makes sense - if it included the Fields that were of the object types as well we might see the circular relationships emerge.

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step (The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before). I think the arrow for most field that have structure or interface are actually visible here.
classDiagram
   Command<|--Linux
   Command<|--Windows
   platformBuilder<|--Linux
   platformBuilder<|--Windows

   class Command{
     <<Interface>>
     +Run() error
   }
   class CrossBuilder{
     <<Interface>>
     +platformBuilder
     +Run()
     -setupContainerImage(flags, args) error
   }
   class platformBuilder{
     <<Interface>>
     +Build(ContainerImage)
   }

   CrossBuilder-->ContainerImage
   CrossBuilder-->ContainerEngine:setupContainerImage() Use
   CrossBuilder-->platformBuilder:Build() Use
   platformBuilder-->ContainerImage
   Command<|--CrossBuilder

   ContainerEngine<|--LocalContainerEngine
   ContainerImage<|--LocalContainerImage
   ContainerEngine<|--KubernetesContainerEngine
   ContainerImage<|--KubernetesContainerImage

   class ContainerEngine{
     <<Interface>>
     +createContainerImage(arch Architecture, OS string, image string) containerImage
   }

   class ContainerImage{
     <<Interface>>
     +Run(vol volume.Volume, opts options, cmdArgs []string) error
   }

   ContainerEngine-->ContainerImage:create

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step

Thanks for this, I think I  am starting to understand.
Proposal: CrossBuilder is not an interface, it is our single struct type that is the core of this package. It would use a PlatformBuilderandContainerEngine` to function.

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.
I hope these ideas are helpful, It should take out some of the seemingly circular usages.

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step

Thanks for this, I think I am starting to understand. Proposal: CrossBuilder is not an interface, it is our single struct type that is the core of this package. It would use a PlatformBuilderandContainerEngine` to function.

It is pretty much this already, except it use a ContainerImage to function which is provided by a ContainerEngine.

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.

I kind of want to avoid touching much of the command part as it will disappear, I believe, once we migrate to cli.App.

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.

I kind of want to avoid touching much of the command part as it will disappear, I believe, once we migrate to cli.App.

I see - but how does the current cli library require that each platform-specific builder rely on the cross builder?

I see - but how does the current cli library require that each platform-specific builder rely on the cross builder?

The cross builder is the provider of Run() which really shouldn't be duplicated, but due to the Cli library requiring the existence of a Run per platform it has to be somewhere. The solution is to either duplicate code (bad) or reuse by depending on cross builder (better). Best will be when we do not need to link the crossBuilder.Run() with the Cli and can then just drop that from the dependencies.

After discussing debugger on the call this is closer but I am still stuck on :

The solution is to either duplicate code (bad) or reuse by depending on cross builder (better).

isn’t there a third option where they don’t have to depend on one another but the common code is used by both?

The solution is to either duplicate code (bad) or reuse by depending on cross builder (better).

isn’t there a third option where they don’t have to depend on one another but the common code is used by both?

I can not think of a different solution until we use cli.App that would be significantly different from any of the one above. I am convinced that using cli.App will solve this problem and that we won't need anymore that duplicated code, but switching to cli.App introduce massive change in the code base and this PR is already big enough that I do not want to do that here. Still I think that that would likely address your problem completely and be your third option.

Please don't force push after reviews started, it now needs a complete review again as we can't see the changes any more

Please don't force push after reviews started, it now needs a complete review again as we can't see the changes any more

Sorry about that. I made a mistake in merging the wrong branch and I had to force push to undo that change.

I had a look at the code and am not sure I agree that crossBuilder must exist because of the way the CLI works.
I see that it removes duplication of shared code, but I don't think it's the right way to do it because crossBuilder and platformBuilder create a tight circular dependency.
Looking at it there are 3 methods, two if which just return the name and description, which is different. The third, Run does all of the hard work. So why not:

return Name and Description back to the commands (platform builder entry points)
move the crossBuilder.Run to a helper method and remove crossBuilder completely
call the helper run method from each platform builder Run

This should be approx the same number of lines as you have, but with a cleaner control dependency graph.

The build failure is unrelated as far as I know and due to a bug with v2.2

Or maybe it's not a bug and this is now using Fyne v2.2.1 tools to compile a Fyne v2.1.4 app - we do ask that folk keep the versions in sync.
Probably we didn't see this coming but it is a side/effect of using the generated code to insert metadata. I guess we could improve the tooling to try and detect if SetMetadata is available.

I have updated the calculator module and kicked the tests so we can see if that was the only error.

Perhaps we should make the current release, then land it to work on with more time?


nim: include all supporting tools

This PR makes a few changes to how things are done:
a) build and install "koch" - the nim make-type tool
b) use "koch" to bootstrap nim
c) build additional supporting tools such as nimble, nimgrep and nimsuggest
d) nim can use other c compilers than gcc, so instead of forcing gcc we use the one from stdenv
e) run the full test suite
We do not need the "nimble" package any longer as it is part of nim
Things done

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

[X] NixOS
[ ] macOS
[ ] Linux

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

FAIL: client.nim --gc:boehm -d:useNimRtl
Test "tests/dll/client.nim" in category "dll"
Failure: reExitcodesDiffer
Expected:
exitcode: 0

Gotten:
exitcode: 1

Output:

PASS: nimrtl.nim -d:release --gc:boehm --app:lib -d:createNimRtl
PASS: server.nim -d:release --gc:boehm --app:lib -d:useNimRtl
FAIL: client.nim -d:release --gc:boehm -d:useNimRtl
Test "tests/dll/client.nim" in category "dll"
Failure: reExitcodesDiffer
Expected:
exitcode: 0

Gotten:
exitcode: 1

Output:

I get some testing failures. Is the version of boehm compatible with nim or does this error only happen on my machine?

All the tests pass here (NixOS).

@Mic92 do the tests fail in the current build if you just add doCheck = true;?

I don't think the tests can run in the current build as they require the "boch" make-style tool which we are only building with this PR.

A sandboxed x86_84 build has finished succesfully, but some tests have
failed in the process without failing the build. I am running an x86
build and there are already test failures and the test run continues, so
I expect the same outcome.

@7c6f434c I actually did not complete the test run.

@Mic92  It is still a clear improvement to have a dozen of ignored test failures compared to being unable to run tests. I thought it breaks the build for you.


fix: events no longer can be comma separated

Same reason as https://github.com/jakewvincent/mkdnflow.nvim/pull/176
After neovim/neovim#25523 the comma separated events are not valid any
more and will throw error

Thanks for the PR!


chore(api): Add local server and GraphQL tests

What's the purpose of this pull request?
This PR adds two new things to improve @faststore/api's development experience:

An Express server setup to the @faststore/api package, so that we can run the API during development without the need to link the package into a store;
An initial set of tests that cover the exported GraphQL schema, queries, and mutations.

How does it work?
There's a new local folder, alongside the actual code for our @faststore/api package, which is not included in the build process. Inside this folder there's a single .ts file that implements an Express server, using express and express-graphql, which serves a single /graphql route at http://localhost:4000/graphql.
When the yarn local command is executed, tsdx gets called to run tsc and watch for changes, then the server is fired up after each successful build.
The tests that validate the GraphQL schema use a function from graphql called assertValidSchema, to make sure the exported schema is valid, and then a few Jest matchers to make sure all expected types, queries, and mutations are actually present. And for both mutations and queries, the execute function, also from graphql is used to actually perform each operation.
How to test it?
To run the Express server in watch mode, run yarn local from inside the packages/api folder.
To run the new tests, run yarn test from inside the packages/api folder.
base.store Deploy Preview
No need, as this is purely a tooling change and should not impact the final bundle.

Very nice! 👏 thank you for adding this. It's going to be very helpful!
Should we add the steps about how to use the local server (http://localhost:4000/graphql) somewhere (Readme, docs)?
I left some comments but not blocking.

Thanks 😄
I don't think it's necessary now that the yarn development command will do this by default.


virtctl,tests: Move imageupload functests

What this PR does
Move the imageupload functests from tests/storage to tests/virtctl to
express their affiliation and ownership.
Before this PR:
imageupload functests live in tests/storage/imageupload.go
After this PR:
imageupload functests live in tests/virtctl/imageupload.go

Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer

Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

[ ] Design: A design document was considered and is present (link) or not required
[ ] PR: The PR description is expressive enough and will help future contributors
[ ] Code: Write code that humans can understand and Keep it simple
[ ] Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
[ ] Upgrade: Impact of this change on upgrade flows was considered and addressed if required
[ ] Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
[ ] Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
[ ] Community: Announcement to kubevirt-dev was considered

Release note

None

@dhiller @brianmcarey Some of these tests have special requirements to storage. Will these tests continue to work with sig-compute lanes?

/lgtm

/lgtm

/approve


ReadME Installation

Hello. Would be very helpful to add a section for how to install.

You add this repo as a dependency to your own project, then you import it into your code.
First, SSH
You might have to set up an SSH key to get xcode to talk to github properly -- don't worry, it looks really involved, but you can mostly copy/paste the example terminal commands they give you
After the SSH stuff
Add this repo as a dependency to your own project

After you have the dependencies setup
Copy his ChartsView.swift into your project so you can see the examples and the preview
Apologies if I've left anything out, I've just gone through the process for the first time myself. I've got it working, best of luck to you


python3Packages.tensorflow(-bin): 2.11.0 -> 2.11.1

Description of changes
2.12.0 is out, but that didn't immediately work for me and 2.11.1 appears to address the same CVEs.
https://github.com/tensorflow/tensorflow/releases/tag/v2.11.1
Seeking volunteer(s) to generate the deps hashes for aarch64-linux ~and aarch64-darwin~, otherwise I'll get ofborg to do it and everyone will hate me for making ofborg build tensorflow...
Things done

Built on platform(s)

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

[ ] For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
[ ] 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

[x] 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/)
23.05 Release Notes (or backporting 22.11 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.

Seeking volunteer(s) to generate the deps hashes for aarch64-linux and aarch64-darwin

On that note, there were some experiments around forcing Bazel to fetch dependencies for all platforms at once in https://github.com/NixOS/nixpkgs/pull/219778/commits/aaf0d0a14fb9acfa5ed02b5dc7536551279646a9. This way there should be just one hash, which can be computed on any platform. I'm still unclear on whether that is actually already working, and I don't know if the same trick could be applied to tensorflow

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/tensorflow-unstable-not-installing-after-update/27041/2

Got the aarch64-darwin hash now

Seeking volunteer(s) to generate the deps hashes for aarch64-linux and aarch64-darwin, otherwise I'll get ofborg to do it and everyone will hate me for making ofborg build tensorflow...

aarch64-darwin sha256-qZvqXi6pvIrZpZdR4BcbOh2C/A4ZHJgXgFINhVJmmcs=
aarch64-linux sha256-g6JUZQQalCTSjvAarkI7+gq13cPhFg/O9LPQDGNvrII=

Needs a rebase before nixpkgs-review can take off

Hmm hopefully #224917 doesn't cause us to have to recalculate all the hashes.

Builds fine on x86_64-linux and works for tts.

Just for the record: we're currently blocked by the merge conflicts

That's because this got bumped past in #225174

That one only updates the -bin?

Damn I shouldn't have combined the two in the same PR. Just confusing. Stand by...

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/tensorflow-unstable-not-installing-after-update/27041/7

Don't forget that this hasa to be backported.

1.10.1 on 22.11 :/

#224988


added descritpion field

while browsing thru scoop buckets I sorely miss description on what is the package supposed to do, so I will try to provide description tags for those

The description property currently isn't used anywhere. So it's only available by looking at the manifest files.
One pull-request for all the changes would have been enough :grin:

that's exactly where I look for a quick glance what is the package about.
forgive me numerous pull requests, I am brand new to the whole github thing

Once most of the apps have descriptions, we can run makeindex.py on them, and generate a summary table, as is shown here: https://github.com/rasa/scoops#apps


Set Default Navigation by Config

Compass, Landmark, etc.

Navigation method will be set based on available hardware and will include more robust options than just compass / landmark.


MSSQL "request.query(sql, callback)" not responding to callback

I'm trying to do a query on my SQL Server, the connection works fine and rises an error when I change the password. The "error" is on request.query(sql, callback), this function is not returning a callback
The connection code:
var sql = require('mssql');

var config = {
user: 'user',
    password: 'password',
    server: 'ip',
    database: 'database',
    connectionTimeout: '5000',
    requestTimeout: '5000',
    options: {encrypt: true}
};

var pool = function(){
    var conn =  new sql.Connection(config, function(err){
        var request = new sql.Request(conn);
        //console.dir(request);
        return request;
    });
    return conn;
}

module.exports = function(){
    return pool;
}

DAO:
function CampanhaDAO(connection){
    this._connection = connection;
    //console.log(this._connection)
}

CampanhaDAO.prototype.getCampanhas = function(){
    var sql = "SELECT * FROM notificacao_campanha";

    this._connection.query(sql, function(err, recordset){

        console.log(recordset);
    });
};

module.exports = function(){
    return CampanhaDAO;
};

Controller:
module.exports.campanhas = function(app, req, res){
    var connection = app.config.dbConnection();
    var campanhaDAO = new app.dao.CampanhaDAO(connection);

    campanhaDAO.getCampanhas(function(error, result){
        console.log("gerou");
        res.send(result);
    });
}

Can anyone help me?

As explained in the documentation you have to listen events :
`const request = new sql.Request();
request.stream = true // You can set streaming differently for each request
request.query(myQueryString) // or request.execute(procedure)
request.on('recordset', columns => {
// Emitted once for each recordset in a query
});`
the same with error, row, done etc..
Doc is here : https://www.npmjs.com/package/mssql#streaming

This demonstrates older documentation issues.
Please consult the updated documentation, particularly around async/await and connection pooling: https://github.com/tediousjs/node-mssql/tree/ee2777b4f088040f83fe87e09ca49b811d3c10f6#examples-1
Closing this issue as I think the documentation has improved enough between then and now.


refactor: refactor the controller manager

What type of PR is this?

/kind refactor
What does this PR do?

Which issue(s) does this PR fix?

Fixes #
Special notes for your reviewer:

Does this PR introduce a user-facing change?

refactor the controller manager

@OrangeBao
We  can go on the enhancement


Resolve #962 - implement --no-copy with tests

Allows types to not have the copy trait automatically derived.

 Warning

These commits modify unsafe code. Please review it carefully!

r?@fitzgen

@bors-servo delegate+

:v: @hallfox can now approve this pull request

@bors-servo r+

:pushpin: Commit f7e370a has been approved by fitzgen

:hourglass: Testing commit f7e370ab2e3a06f2e7776dc8c1b3df3c49891e08 with merge 716cec7ba7f50848d1546d0cd9d987fb21afb435...

:sunny: Test successful - status-travis
Approved by: fitzgen
Pushing 716cec7ba7f50848d1546d0cd9d987fb21afb435 to master...


Wrong PDF bookmarks hierarchy

Prerequisites

[ X] Put an X between the brackets on this line if you have done all of the following:

Check that you are using the latest version of kaobook.
Check that your issue isn't already filed: https://github.com/fmarotta/kaobook/issues

Description
The change introduced in issue #101 has introduced an undesired side effect and now the PDF Bookmark hierarchy is incorrect. You get something like (with certain elements duplicated and weird nesting):

Instead of the correct one:

As I am not using the minitocs, a quick fix that worked for me was just add these to the preamble of my document:
\renewcommand{\mtocsection}[1]{}
\newcommand{\mtocsubsection}[1]{}

But a proper solution should be found for those that use the minitocs.
Additional Information
I have already written a comment on issue #101, but then I have realized that opening an issue was more appropriate.

I have identified what was causing the problem. In issue #101, the suggested definition for commands \mtocsection and \mtocsubsection was:
\newcommand{\mtocsection}[1]{
    \etoctoccontentsline{mtocsection}{\protect\numberline{\thesection}#1}
}
\newcommand{\mtocsubsection}[1]{
    \etoctoccontentsline{mtocsubsection}{\protect\numberline{\thesubsection}#1}
}

But in kao.sty these were incorporated as:
\newcommand{\mtocsection}[1]{
    \addcontentsline{toc}{mtocsection}{%
        \ifnum\value{secnumdepth}>0 \protect\numberline{\thesection}%
        \else \protect\nonumberline%
        \fi #1}%
}
\newcommand{\mtocsubsection}[1]{
    \addcontentsline{toc}{mtocsubsection}{%
        \ifnum\value{secnumdepth}>1 \protect\numberline{\thesubsection}%
        \else \protect\nonumberline%
        \fi #1}%
}

As suggested in etoc documentation, \etoctoccontentsline must be used instead of \addcontentsline to circumvent the patching for  automatic creation of bookmarks
done to \addcontentsline by the hyperref package.
The problem with the wrong PDF hierarchy is solved if thee commands are defined as (although I do not know if the change will have any other impact in other parts of the document class):
\newcommand{\mtocsection}[1]{
    \etoctoccontentsline{mtocsection}{%
        \ifnum\value{secnumdepth}>0 \protect\numberline{\thesection}%
        \else \protect\nonumberline%
        \fi #1}%
}
\newcommand{\mtocsubsection}[1]{
    \etoctoccontentsline{mtocsubsection}{%
        \ifnum\value{secnumdepth}>1 \protect\numberline{\thesubsection}%
        \else \protect\nonumberline%
        \fi #1}%
}

I think with this the issue could be close, but I prefer the author of the package closes it after incorporating the changes to kao.sty.

Not working here. Getting
ERROR: Missing number, treated as zero.

--- TeX said ---
<to be read again>
\Etoc@level
l.217 \chapter{Blah blah}

Thank you very much, I have implemented the changes you suggested.


How not to return 403 error code.

So I have a usecase, where I don't want to return 403 access code, I am only interested in the log, that an SQLi attack has been detected, so that I can ingest that log in SIEM.

For a specific rule you'll have to modify the rule. If you don't want to block anything, you can simply set the engine to DetectionOnly. That will generate the logs without blocking anything.

Thanks for the help


why the back of model is Defect？

Description
there is many models, but only this model is abnormal,is it caused by doublesided?
sketchfab is normal,but abnormal in this viewer
my viewer:

sketchfab

Live Demo

https://sketchfab.com/3d-models/shell-harpago-chiragra-3e5309f1a099400aa1ce79b449dfdf4b
https://testing.modelvue.growlib.com/index.html?url=https://growlib-model-sk-hk.oss-cn-hongkong.aliyuncs.com/play/103485/scene

Browser Affected

[ ] Chrome
[ ] Edge
[ ] Firefox
[ ] Helios
[ ] IE
[ ] Safari

OS

[ ] Android
[ ] iOS
[ ] Linux
[ ] MacOS
[ ] Windows

Versions

model-viewer: vX.X.X
three.js: rXX

@braveheartty I'm not getting a repro; your demo renders just fine for me on Chrome Linux. Would you mind filling out the form below your post so we know what platform and version is having trouble? If it's a mobile device, please be specific about the model as sometimes things like this are driver bugs.


Add confusion matrix with some of ROC metrics.

A confusion matrix would be helpful for exploring differences in balanced and unbalanced sets for classification and/or training. It would be very helpful for understanding what kind of metric is good for specific problems.

Well, it might be useful. But I would first focus on having everything for simple binary classification. And then we can build up with this. From my experience, I rarely use more then 2 classes for semantic segmentation. Few times I used 3 classes (background, object, edges of objects).


[ doc, build_doc ] Optionally bind to global wan when previewing

This commit adds an additional --bind-wan argument to build_docs.py.
This forces the hugo server to bind the http server a port on the global
interface (as opposed to just on localhost, which is the default).
This is important when using hugo on build farms with older (or no) browser
support, and no support for ssh tunneling.
Signed-off-by: Martin Lueker-Boden<EMAIL_ADDRESS>

Thanks @asb for suggesting this!

Thanks Martin. I am not sure what the use case is for you. Will you run the web server on a remote server and want to browse the documentation from your machine? Does this work for you? I tried this at my side but there are some problems, first, redirects with aliases are not working as they still point to localhost and second, the reference to 'style.css' is also done with localhost, so it is not a pleasant experience. But the text is there, so for terminal based browsing it might be okay ;) But it might be just a problem with my setup...

I think the other implementation approach that's worthy of consideration is to provide some kind of "--hugo-args` option (or an environment variable) to allow extra hugo arguments to be passed in a flexible way, on the basis that there might be argument beyond this one that are helpful. This use case could be listed as an example in that documentation.
Does anyone have strong feelings one way or another? Per @towoe's comment, I wondered if you need to have --baseURL set to something appropriate too for this to work as expected?

Thanks Martin. I am not sure what the use case is for you. Will you run the web server on a remote server and want to browse the documentation from your machine? Does this work for you? I tried this at my side but there are some problems, first, redirects with aliases are not working as they still point to localhost and second, the reference to 'style.css' is also done with localhost, so it is not a pleasant experience. But the text is there, so for terminal based browsing it might be okay ;) But it might be just a problem with my setup...

Hi @towoe,
Interesting.  Maybe there is some subtlety I'm missing, but thus far it looks pretty good on my end.  If you have time it would be great if you could send a screenshot to illustrate your problems.  However maybe @asb is right that there are some more arguments I could add to straighten this out.
As far as use case, you have it right.  We are always developing on a remote compute farm, because that is where our tool licenses are for our proprietary machines (and frankly, that is just how our IT folks want to support most us)  There are some options for viewing, such as:

Launching a remote browser, which is clumsy at best, especially since the browsers are old. Worse still though, this is not working for some users for some system reason I don't understand.
ssh tunnels to connect the "browser localhost" to the "server localhost".  This works on many Linux systems, but direct ssh is disabled on our compute farm.  So this option requires us to have a split workflow (proprietary tools on one host, documentation on another), which is suboptimal.
Linux VM's just for browsing doc-edits, also suboptimal.
The solution outlined here.

As an aside, this is actually the last hiccup to having a completely working OpenTitan flow on our RHEL6 compute farm servers.  Many of the other fixes required doing strange things like creating custom toolchains (both native and RISCV) or using using patchelf, to point newer binaries (like hugo-extended) to a more modern version of glibc.

I think the other implementation approach that's worthy of consideration is to provide some kind of "--hugo-args` option (or an environment variable) to allow extra hugo arguments to be passed in a flexible way, on the basis that there might be argument beyond this one that are helpful. This use case could be listed as an example in that documentation.
Does anyone have strong feelings one way or another? Per @towoe's comment, I wondered if you need to have --baseURL set to something appropriate too for this to work as expected?

Hi @asb, this is actually a great idea.  I could actually continue to tweak our setup without causing to much churn on the main repo.  Thanks, I'll try that.

Btw, you should be able to commpile Hugo yourself on RHEL6 and use it there; it's just the pre-compiled version of Hugo that doesn't work.

I think the other implementation approach that's worthy of consideration is to provide some kind of "--hugo-args` option (or an environment variable) to allow extra hugo arguments to be passed in a flexible way, on the basis that there might be argument beyond this one that are helpful. This use case could be listed as an example in that documentation.
Does anyone have strong feelings one way or another? Per @towoe's comment, I wondered if you need to have --baseURL set to something appropriate too for this to work as expected?

As far as args vs. environment variables, environment variables are super convenient, but I often worry about environment contamination causing trouble for other users who aren't aware of them.   I might actually do something like this: make a variable HUGO_ARGS, but then ignore it unless there is a flag --env-hugo-args.   Any thoughts on that?
If that is too baroque, then a simple --hugo-args (and a wdc-side script that adds the right args) might be fine.

Btw, you should be able to compile Hugo yourself on RHEL6 and use it there; it's just the pre-compiled version of Hugo that doesn't work.

Yeah, but compiling was a problem on it's own for the tool and dependency setup.  It might get easier as we make progress with our native toolchain.  In the meantime patchelf was far far easier (though I did have to use the right version so maybe that is not the best long term solution).  I'll probably be writing a footnote doc to link to the software page.

I just gave it a try and compiled hugo on RHEL6, you can download the binary here: https://drive.google.com/file/d/1eLGchH-aX52M2pxuGFmSt6aeGZMPLxzN/view?usp=sharing
If that works for you we can modify the build_docs.py script to use this build by default when running on RHEL6, providing users on RHEL6 the same experience others.

I just gave it a try and compiled hugo on RHEL6, you can download the binary here: https://drive.google.com/file/d/1eLGchH-aX52M2pxuGFmSt6aeGZMPLxzN/view?usp=sharing
If that works for you we can modify the build_docs.py script to use this build by default when running on RHEL6, providing users on RHEL6 the same experience others.

Thanks @imphil!  I can give it a try.

@towoe: Interestingly enough I now see what you are talking about.  (I seem to have some hysteresis in my page views, presumably due to browser caches?)   Though @asb, your baseURL suggestion seems to work very well.   I added baseURL argument into the settings added by the --bind-wan option.
@towoe, could you try it now?
As for the more generic approach (--hugo-opts or environment variables), I added that too, and that should help for others wanting to add additional options.   However for the simple case, the --bind-wan option is a bit easier to use as it requires less scripting on the part of the user (have to look up the hostname etc.) and it would be nice to not need to make another script to wrap this script.   So I've left them in.
I played with the option of environment variables, however that is not quite compatible with our environment management system.  On this compute farm we are often jumping from node to node, and because of some peculiarities of the environment management tools on our system keeping the "baseURL" argument in an environment variable could cause confusion.
That said, I'd like to add both the --hugo-opts and --bind-wan flags.

BTW: Thanks for the review @swarbrick.   Does anyone else have any thoughts or reservations on merging this as is with these two new options?  I ask because we are having active conversations right now about our remaining capability gaps on the WDC side servers, and this is one of the few remaining items.
(BTW it is really exciting to be able to say that, I really appreciate the help from everyone who helped us get this far!  I think @imphil in particular deserves a big shout out, for all his support builds of various tools.)

LGTM if it does the trick for you!
About the hugo binary: did the binary I linked to work for you? And if so, do you want us to distribute it somewhere for you to install locally, or do you have ways to produce a working hugo binary for RHEL6 yourself (and make it available e.g. through module load)?

Thanks @imphil!  You bring up a good point though, I'll try to get at least one more person to try this on our system (to make sure I'm not missing something else).

Thanks all.  I've had a couple people test the latest version from our side, and aside from a minor bug (--bind-wan without --preview crashed... fixed) it seems to work well. Merging..


Fails to compile: Obsolescent feature: DO termination statement which is not END DO

When compiling the current version I get the following error
[  1%] Building Fortran object src/CMakeFiles/fortran_utils.dir/optimize.f90.o
[  2%] Building Fortran object src/CMakeFiles/fortran_utils.dir/lapack.f90.o
[  3%] Building Fortran object src/CMakeFiles/fortran_utils.dir/linalg.f90.o
[  3%] Building Fortran object src/CMakeFiles/fortran_utils.dir/mesh.f90.o
[  4%] Building Fortran object src/CMakeFiles/fortran_utils.dir/ppm.f90.o
[  5%] Building Fortran object src/CMakeFiles/fortran_utils.dir/sorting.f90.o
[  6%] Building Fortran object src/CMakeFiles/fortran_utils.dir/special.f90.o
/Users/carvajal/fortran-utils/src/special.f90:565:25:

 10            DJ(K)=0.0D0
                         1
Error: Obsolescent feature: DO termination statement which is not END DO or CONTINUE with label 10 at (1) [-Werror]

Thanks. I think I should put those special functions into a special .f file and compile that file with legacy Fortran.

Do you mean to change the special.f90 to special.f or move the functions inside to a separate file?

Just move the F77 functions that I took from SciPy to a separate file. I thought you can compile them as part of a modern Fortran code, but this error shows that really you can't.

@certik  shall I do that and send a pull request?

@krvajalmiguelangel that would be great. I am busy today to do it myself.

All you need is to add some end do's like so:
           DO K=0,N
              SJ(K)=0.0D0
              DJ(K)=0.0D0
           END DO

@jacobwilliams I already sent a pull request and it got fixed. Shall I close the issue @certik ?

@krvajal that would be great. And indicate which PR fixed it.

@certik turns out that just moving the source code to a separate file with the .f extensions do not fix the issue because the same flags are been used for all Fortran files, this flag is -std=f2008 and makes the warnings to appear.
One possible solution is to fix the DO LOOPS by removing the tag and adopting the new style or moving the files with the .f extension to a separate directory and use the -std=legacy option when compiling them.
Please let me know which direction you like the most.

@krvajal in other projects I simply fix this in cmake to compile F77, e.g.: https://github.com/certik/minpack/blob/b3ea6c29debe689a20cb6884cce9b09dce6d0a00/src/CMakeLists.txt, and one can do that on a per file basis as well.

I am getting the exact same error as above. Did a fix for this get implemented? I pulled the latest version of fortran-utils just yesterday.
[  2%] Building Fortran object src/CMakeFiles/fortran_utils.dir/types.f90.o
[  4%] Building Fortran object src/CMakeFiles/fortran_utils.dir/utils.f90.o
[  4%] Building Fortran object src/CMakeFiles/fortran_utils.dir/amos.f90.o
[  4%] Building Fortran object src/CMakeFiles/fortran_utils.dir/constants.f90.o
[  7%] Building Fortran object src/CMakeFiles/fortran_utils.dir/optimize.f90.o
[  9%] Building Fortran object src/CMakeFiles/fortran_utils.dir/lapack.f90.o
[ 12%] Building Fortran object src/CMakeFiles/fortran_utils.dir/linalg.f90.o
[ 12%] Building Fortran object src/CMakeFiles/fortran_utils.dir/mesh.f90.o
[ 14%] Building Fortran object src/CMakeFiles/fortran_utils.dir/ppm.f90.o
[ 17%] Building Fortran object src/CMakeFiles/fortran_utils.dir/sorting.f90.o
[ 19%] Building Fortran object src/CMakeFiles/fortran_utils.dir/special.f90.o
/Users/nsgates/Documents/fortran-utils/src/special.f90:565:25:

 10            DJ(K)=0.0D0
                         1
Error: Obsolescent feature: DO termination statement which is not END DO or CONTINUE with label 10 at (1) [-Werror]

@krvajal, @nsgates things should be fixed now. If anything does not work, let me know.


Package blacklisted on roave/security-advisories

https://github.com/Roave/SecurityAdvisories is a repository to block packages which are insecure. The version constraint of this package is: "erusev/parsedown": "<=1.6.4",, so it is not possible to install this package with security-advisories active. Whats the reason for the insecurity? I din't see any commits about it.

Hey
https://github.com/erusev/parsedown/search?q=xss&type=Issues&utf8=✓

If you up your composer version to "erusev/parsedown": "^1.7.0", or even just run composer update this should now be resolved :)


Docs misrendering on development

Current HEAD version: https://lakefs-spec.org/0.3.1.dev14+gbc76a28/tutorials/demo_data_science_project/
I see the following problems:

[ ] Bulleted markdown lists in the beginning are not rendered,
[ ] Pandas DataFrame prints are written over the line.

If we decide to keep the output cells for the docs build, we should also add some information to the remaining print statements, just as we did to the data shapes and accuracies.

The lists in Markdown cells render correctly for me when adding a blank line before the first list item:Foo:

* Bar

Instead of:
* Bar

Data frame rendering could be improved with a bit of custom CSS (ping me if you need help): https://github.com/greenape/mknotebooks/issues/24


Refactor Promise classes with generic types

Q
A

Bug fix?
no

New feature?
yes

BC breaks?
no

Deprecations?
no

Related tickets
fixes #23

Documentation
see below

License
MIT

What's in this PR?
This PR refactors the Promise interface and concrete classes to use generic types. This allows to hold a meta-reference to the type of value the promise will resolve to. The then method will return a separate template type, so you can actually build properly typed then-chains.
For the Promise template, a covariant template type has been introduced. This allows library authors to create their own, constrained Promise types (say, UserPromise<U extends User>).
All in all, these annotations will improve type safety in a lot of code bases.
Regarding the docs
I'm not quite sure whether these additions should be added to the documentation (probably). WDYT?
Example Usage
I'm copying the (contrived) example from #23 here:
class PromiseFactory
{
    /**
     * @template T
     * @param T $value
     * @return Promise<T>
     */
    public static function make(mixed $value): Promise {
        return new SomePromiseImplementation($value);
    }
}

/**
 * @return Promise<string>
 */
function foo(): Promise {
    return PromiseFactory::make('test');
}

// Inferred to be a string
$stringValue = foo()->wait();

// Static analysis tools will be able
// to figure out all types involved here!
$floatValue = foo()->then(fn($value) => parseFloat($value))->wait();

Checklist

[x] Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
[ ] Documentation pull request created (if not simply a bugfix)

To Do

[ ] Discuss documentation changes

i fixed the cs issue in the master branch, please rebase.
the documentation for promise is managed in the documentation repository: https://github.com/php-http/documentation/blob/main/components/promise.rst
however we don't talk about implementing your own promises. if you want to add a section at the end titled "Implementing your own Promises" or something, you could explain it there. on the other hand, i think for people used to generics, its kind of self-explanatory.

i fixed the cs issue in the master branch, please rebase.

👍

however we don't talk about implementing your own promises. if you want to add a section at the end titled "Implementing your own Promises" or something, you could explain it there. on the other hand, i think for people used to generics, its kind of self-explanatory.

Yeah, that's was I was wondering. Let me give it a shot, maybe a single paragraph on generic type support is enough to simply let people know they can use it if they like?

wrapped up in #27, thanks @Radiergummi !


Change default highlighted text colour

System: Darwin 19.4.0 (x86_64)
Compiler: g++ 4.2.1 Compatible Apple LLVM 11.0.0 (clang-1<IP_ADDRESS>)
ncurses: ncurses 5.7.20081102 (compiled with 5.7)
libcurl: libcurl/7.64.1 (SecureTransport) LibreSSL/2.8.3 zlib/1.2.11 nghttp2/1.39.2 (compiled with 7.64.1)
SQLite: 3.31.1 (compiled with 3.31.1)
libxml2: compiled with 2.9.4
I am aware that colours can be configured, but the default yellow-on-cyan colour scheme is a bit too bright on (at least) my configuration on iTerm2. Cyan background is fine, but changing the default text colour to black or at least something darker would help with readability a lot.

Steps to reproduce the issue:

Open newsboat

There's no knowing what colorscheme user's terminal would use. Optimizing defaults for your colorscheme will break someone else's default look, so we simply use what other similar programs use.


Bern features: Changing affiliation form "Belongs to a series" to "Belongs to this video" does not work

When I change the affiliation of a category from "Belongs to a series" to "Belongs to this video" the category and its labels are still shown and can be selected in all other videos of this series. Of course I did a browser reload inbetween. The change the other way around (belongs to video > belongs to series) does work.
Solution: When a user/admin changes the affiliation of a category form "Belongs to a series" to "Belongs to this video" the chosen category should only be offered in the video where this change was made (regardless of which video the category was created in). In all other videos the category, that now only belongs to a certain video, should disappear.

To reiterate, what currently happens (or should happen at least) when a category is changed back to "Belongs to this video" from "Belongs to a series", this happens on all videos the category ever existed on. For example, if the category is changed back to a video category on video A, it will also do so video B.
This is currently intended behaviour, to avoid a user unexpectedly deleting categories from other videos where those categories are maybe still in use. Your solution would also be valid behaviour though. I simply do not know enough about the intended use cases of the feature to make an educated decision on how the feature should behave in this case. If you are sure that your solution is the intended way of how this should work, we can implement that.
In that case it might be a good idea to display a modal with a warning message, i.e. something like "Changing the affiliation of this category from series back to video will delete the category from all other videos of this series!".

Thx for your explanation. I like your idea to display a modal with a warning message. Also the warning message seems appropriate. I am pretty sure that we should implement my solution, although I do not know all usecases of this feature. But I want to prevent the case, that someone does switch a category to belonging to a series by mistake and after having changed it back is forced to delete all unwanted "copies" in the other videos. Does that sound reasonable to you?

I agree with you, having to delete a bunch of accidentally created categories on different videos would be very bothersome. Therefore I will try and implement your suggested solution, along with the warning modal I mentioned to avoid accidental deletion on the users part.

Solved with commit https://github.com/opencast/annotation-tool/pull/414/commits/614c0136e91f4ef861bf63a604f8408f10aade58 and https://github.com/opencast/annotation-tool/pull/414/commits/d3187a5815ef7027ebe89d4268010f7f3055b168.

Solved with commit https://github.com/opencast/annotation-tool/pull/414/commits/614c0136e91f4ef861bf63a604f8408f10aade58 and https://github.com/opencast/annotation-tool/pull/414/commits/d3187a5815ef7027ebe89d4268010f7f3055b168.

Please alter german warning message.
 Wollen Sie fortfahren?"
Button: "Ja, zu Video-Kategorie machen"

Please alter german warning message.
 Wollen Sie fortfahren?"
Button: "Ja, zu Video-Kategorie machen"

Looks fine, thx!

Looks fine, thx!


improve exception messages and verify encryption output

#15
Also added a verification step during encryption to make sure the output will always be a valid string.
All errors because of the inputs will now be emitted as onError(CryptoDataException) and not throw a fatal Exception anymore.

Looks good. Thanks!