fix: The intro text on private notes list stays fixed

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/49298
PROPOSAL: https://github.com/Expensify/App/issues/49298#issuecomment-2353845750
Tests

Open 1:1 chat
Click on the header
Add private notes with many lines
In private notes list page, scroll the page
Verify that: The intro text should be attached to the content of the page and scroll like

[x] Verify that no errors appear in the JS console

Offline tests

QA Steps

Open 1:1 chat
Click on the header
Add private notes with many lines
In private notes list page, scroll the page
Verify that: The intro text should be attached to the content of the page and scroll like

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
[x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))

[x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
[x] If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:

[x] I verified that all the inputs inside a form are aligned with each other.
[x] I added Design label and/or tagged @Expensify/design so the design team can review the changes.

[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

https://github.com/user-attachments/assets/a94b4bb6-fdf7-4d1d-816f-bb88da397905

iOS: Native

https://github.com/user-attachments/assets/6a9e08a7-445d-4258-9497-22ccc03a2257

iOS: mWeb Safari

https://github.com/user-attachments/assets/873128ae-370f-4f28-9cfa-5843a49bb0ea

MacOS: Chrome / Safari

https://github.com/user-attachments/assets/99e510eb-e6bd-4c08-923a-79baba76b388

MacOS: Desktop

https://github.com/user-attachments/assets/924319a1-7ac0-41ea-9b56-e7496fd04256

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[x] I checked that screenshots or videos are included for tests on all platforms
[x] I included screenshots or videos for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

[x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[x] I verified proper code patterns were followed (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I verified that this PR follows the guidelines as stated in the Review Guidelines
[x] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)

[x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
[x] If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:

[x] I verified that all the inputs inside a form are aligned with each other.
[x] I added Design label and/or tagged @Expensify/design so the design team can review the changes.

[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
[x] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

https://github.com/user-attachments/assets/1276606c-2b08-4648-ac6b-3000eec47bf9

Android: mWeb Chrome

https://github.com/user-attachments/assets/6f1105f0-f178-4abd-ab77-0f006e67528a

iOS: Native

https://github.com/user-attachments/assets/b48c2d8b-4ce6-4f18-9e5e-dc70f6a1c7a5

iOS: mWeb Safari

https://github.com/user-attachments/assets/e31933d2-71b2-466f-92f3-0c85a69d18a4

MacOS: Chrome / Safari

https://github.com/user-attachments/assets/8be64839-f0b0-4f3f-bc93-8bab9ca3d509

MacOS: Desktop

https://github.com/user-attachments/assets/c020327b-8378-426c-bfbc-fb2db5a6ac31

I noticed some strange behavior when trying to select all text inside the input field (shown in my Android Web video). This wasn't introduced or fixed by this PR; I see the same issue in my latest main:
https://github.com/user-attachments/assets/bc67dd97-5a43-44e9-9496-47d82887dab8
I'm unsure if it's an issue with our app or my local environment, but I'll investigate further. Thanks!

Thanks!

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.


I have this mod late in my load order, with no install of SLA or O variants. I find the actor being naked stops 'viewing naked actors' or 'erotic armour' having any influence on the baseline arousal. It is just the 30 baseline. Wearing something in the body slot causes the baseline to go to 40 (20 viewing + 20 erotic armour). Being naked seems to override everything else.

if (isNaked)
{
	libidoModifier += settings->GetNudeArousalBaseline();
} else if (Utilities::Actor::IsViewingNaked(actorRef)) {
	libidoModifier += settings->GetNudeViewingBaseline();
}

Shouldn't these be both their own "IF" statements rather than having an ELSE-IF? Could this be why Nudity is overriding?
if (!isNaked) {
	if (const auto eroticKeyword = settings->GetEroticArmorKeyword()) {
		const auto wornKeywords = Utilities::Actor::GetWornArmorKeywords(actorRef);
		if (wornKeywords.contains(eroticKeyword->formID)) {
			libidoModifier += settings->GetEroticArmorBaseline();
		}
	}
}

I think you need to add a statement for the actor being naked and viewing erotic armor? or otherwise being naked (No Slot32 in vanilla) while still having the erotic armor keyword in another slot?


SwiftUI intrinsic content height

Description
i've been using PanModal to present SwiftUI modals on top of UIKit views for a while, and it's been working great! i'm noticing an issue that occurs when the height of my SwiftUI view changes, when using a pan modal height of .intrinsicContentSize, recalculating the height is

non-deterministic: as in recalculations of the same SwiftUI height result in different final UIKit heights
and requires a DispatchQueue.async, presumably to wait for the SwiftUI layout to finish before UIKit calculates sizeThatFits, but I'm unsure

here's some sample code for how I setup my PanModal view
class ToyPickerController: UIHostingController<ToyPickerView> {

    init(didTapClose: @escaping () -> Void, didSelect: @escaping (Toy, Toy.Option?) -> Void) {
        super.init(rootView: ToyPickerView(didTapClose: didTapClose, didSelect: didSelect))
        self.rootView.invalidateHeight = { [weak self] in
            DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(10)) { // <- RELEVANT
                self?.panModalSetNeedsLayoutUpdate()
                self?.panModalTransition(to: .shortForm)
            }
        }
    }

}

extension ToyPickerController: PanModalPresentable {
    var panScrollable: UIScrollView? { .none }

    var shortFormHeight: PanModalHeight { .intrinsicHeight }
    var longFormHeight: PanModalHeight { .intrinsicHeight }

    var cornerRadius: CGFloat { 24 }

    var showDragIndicator: Bool { false }
}

I call invalidateHeight() right after I make a change that will cause the height to become different. In the following example, it's when I click on the puzzle button
and here's a screen recording showing non-deterministic height for the view: you can see I change the height of the modal after presenting it 3 times in succession: the first time the height decides to shrink, the second time it grows, and the third time it stays the same. In SwiftUI, the heights of both states are the same
https://user-images.githubusercontent.com/3814772/108647668-1b68a700-746e-11eb-8237-ba5f3b868e05.mp4
i'd love to figure out how to remove the DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(10)), and maybe that will solve the non-deterministic intrinsic content size as well? happy to provide more code/create an example. thanks!

rationale was to give UIKit enough time to layout to it's intrinsic size before PanModal asks the view for it's height


Remove file extension

Description
Fixes failing build
Type of change
Please delete options that are not relevant.

[x] Bug fix (non-breaking change which fixes an issue)

Screenshots
Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation

pnpm next-build

What error are you getting @kilemensi ? Could it be a caching issue?

Something about styles.scss @koechkevin .
... but cache could be the issue. Let me pnpm clean first and see.


Issue 207 - Product File Detail API

Added a new endpoint /products/X/, where X can be either a model id or a file name.
Response includes the product model details and lists of its associated sources, ancestor products, and descendant products.

Closes #207

LGTM, I'll go ahead and merge.


Add type check for boolean expect.

I tried this and it should fix #347, but let me know if you would want to see more tests added here.

Closing. See in https://github.com/antonmedv/expr/issues/347#issuecomment-1461952794


thread-safe

:warning: Please install the  to ensure uploads and comments are reliably processed by Codecov.
Codecov Report
Attention: Patch coverage is 96.77419% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.99%. Comparing base (34acce8) to head (0f3b23c).

Files
Patch %
Lines

test_threads.c
96.36%
1 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
- Coverage   97.18%   96.99%   -0.20%
==========================================
  Files           2        3       +1
  Lines          71      133      +62
  Branches       11       22      +11
==========================================
+ Hits           69      129      +60
- Misses          1        2       +1
- Partials        1        2       +1

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


1.0.0 version broken with asdf - deno.gz: not in gzip format

Getting the following output when trying to install v1.0.0 with asdf:
$ asdf install deno 1.0.0
∗ Downloading and installing deno...

gzip: /home/jeanlescure/.asdf/installs/deno/1.0.0/bin/deno.gz: not in gzip format
Fail: Could not uncompress
Fail: An error occurred

@jeanlescure I think this issue should be moved to this repo: https://github.com/asdf-community/asdf-deno

Went ahead and created the issue in the proper repo. Thanks @MarkTiedemann :+1:

Thanks, @jeanlescure!
Since the root cause is fixed now, do you want to close this issue?

Sure thing


[DPEDE-328] Create common dropdown menu items internal component

Can we try to use the current components from UxPin as <Checkbox> and <Radio>?

I believe using them will create some complexity as we need to dynamically declare prop names like options for radio and label, checked properties for checkbox.


Failing Request

In cases where starting a request is not possible (e.g. base url for BaseUrlKey is missing), requests must be able to fail.
One way would be to have an optional network task as return type.

May be replaced by #34

done with #35


Improvements to JSON format of web module

use JSON request instead URL parameter
different APIs of the Directions API should use same time unit: seconds (currently routing API has ms)
different layout for via-points? 'legs', what about changing the vehicle?
turn_angle is in radian
Also see #439 as some properties could be per edge not per instruction
remove insecure JSONP?
change default for GPX: export only track?
use different wording for points e.g. use geometry?
The input point should be better named e.g. waypoint then one can easily use waypoints for the output JSON name, see #727
use waypoints_order instead of points_order
way_point_max_distance clashes with max_waypoint_distance see #872
use different wording for paths e.g. routes?
info endpoint should be improved, either via profile_x:{} (and remove features) or a different layout
make input and output points more consistent all using [lon,lat,elevation] and output using GeoJSON? Same applies to the polyline encoding: the encoded string for points and snapped_waypoints has the order lat,lon,elevation to make it compatible with other clients but we should use lon,lat,elevation
remove 'took' from output, see #457
vehicle vs. profile (e.g. foot is no vehicle)
make i18n response more consistent or remove the need for a separate endpoint

use JSON request instead URL parameter

Are there any examples where this is considered a good idea? I thought the whole point about REST was that a resource is identified by its URL alone, and a shortest path is the perfect example of a resource.
navitia.io seems to agree.

Are there any examples where this is considered a good idea?

ElasticSearch, our Route Optimization API, mapzen, ...

I thought the whole point about REST was that a resource is identified by its URL alone, and a shortest path is the perfect example of a resource.

Why? There are lots of possible parameters. E.g. it is time, vehicle size etc dependent. I do not see it as a resource. I see it more as a 'calculation job'...

Why?

Because it doesn't change the world, because it returns the same result until it is explicitly changed by something, because it can be cached, ...
Google search is GET

remove insecure JSONP?

+1
Not only because security, but also because we are not a web framework.
I think in a few years we will say "Oh, you want our API a bit different? Go use a web tool to click together a proxy/gateway which changes it like you want it."

because it returns the same result until it (the road network) is explicitly changed by something, because it can be cached

I would argue against this (as real time road changes will invalidate this and it is unclear which routes to invalidate), but I think this is indeed discussible :)
Let me give you one "beauty" argument as why we shouldn't care much about principled web stuff here:
we'll be able to configure a CH vehicle profiles with a JSON file. Also the default vehicle profiles for non-CH should be configurable by JSON and now, we are forced due to beauty reasons ;), to use the same JSON to request our API to dynamically change the vehicle profile per request.

And, of course, as soon as you start thinking about subscriptions to routes

Yes, but then you could also argue about a resource that you subscribe to ... again: I wouldn't care much. BTW: navitia api is reeaally old ;)

we'll be able to configure a CH vehicle profiles with a JSON file. Also the default vehicle profiles for non-CH should be configurable by JSON and now, we are forced due to beauty reasons ;), to use the same JSON to request our API to dynamically change the vehicle profile per request.

I can't follow. Not even which of these is supposed to be good or bad.
So you want to use the same JSON description of a vehicle profile, no matter if you are

creating a pre-computed profile
giving parameters to the flexible router

Correct?

Or did you mean that we do not want this, because it would be overdoing it? But then I don't see the connection to POST with JSON, because then we can use JSON for configuring the pre-computation  and URL parameters for the request and nobody cares.

So you want to use the same JSON description of a vehicle profile, no matter if you are

Yes, I meant this and I think this as a good thing :) There is also a third use case where you change defaults e.g. of car to toll=no or ferry=no etc.


Many links are not working

The following links are not working and the concepts are also not available. Is it under progress?

Objects and Data Structures
Classes

S: Single Responsibility Principle (SRP)
O: Open/Closed Principle (OCP)
L: Liskov Substitution Principle (LSP)
I: Interface Segregation Principle (ISP)
D: Dependency Inversion Principle (DIP)

Don’t repeat yourself (DRY)

Closing this as it has been resolved in #4


[feat] distributed captioning with Qwen

Known gotchas:

Not all models listed in https://docs.vllm.ai/en/latest/models/supported_models.html will have the same format for doing video captioning. Chane the recaption.py as needed to suit your needs.
limit_mm_per_prompt needs adjustment based on the model being selected.

After I ran launch.sh, I got:
ls -lh $output_dir/*.txt

...
-rw-rw-r-- 1 sayak sayak  596 oct.  15 11:42 video-dataset-disney/d3af95bf8f9cce29e5c99e839e630c59_caption.txt
-rw-rw-r-- 1 sayak sayak  596 oct.  15 11:42 video-dataset-disney/d8a33062ef6a446c168acb06bf45b77d_caption.txt
-rw-rw-r-- 1 sayak sayak   55 oct.  15 11:42 video-dataset-disney/ec80a7b9cfb3740e5cdf9300fdc8d8a9_caption.txt
-rw-rw-r-- 1 sayak sayak  647 oct.  15 11:42 video-dataset-disney/f6a95cd4397b6a102e82becdcd05d585_caption.txt
-rw-rw-r-- 1 sayak sayak  591 oct.  15 11:42 video-dataset-disney/fe0efaedf8c47812bff8da9951f77975_caption.txt
-rw-rw-r-- 1 sayak sayak  414 oct.  15 11:42 video-dataset-disney/fec5a5184b05acafc7904cd419cbb5a3_caption.txt

@a-r-r-o-w this is ready for reviews.

awesome, testing now!

Good to take note of by the way: https://github.com/PKU-YuanGroup/Open-Sora-Plan/blob/main/docs/Report-v1.3.0.md#captioning

I get the following error when launching. Any idea why?

 stacktrace
(nightly-venv) aryan@hf-dgx-01:/raid/aryan/cogvideox-distillation/video_recaptioning$ ./launch.sh
WARNING 10-17 12:43:21 cuda.py:76] Detected different devices in the system:
WARNING 10-17 12:43:21 cuda.py:76] NVIDIA A100-SXM4-80GB
WARNING 10-17 12:43:21 cuda.py:76] NVIDIA A100-SXM4-80GB
WARNING 10-17 12:43:21 cuda.py:76] NVIDIA A100-SXM4-80GB
WARNING 10-17 12:43:21 cuda.py:76] NVIDIA DGX Display
WARNING 10-17 12:43:21 cuda.py:76] NVIDIA A100-SXM4-80GB
WARNING 10-17 12:43:21 cuda.py:76] Please make sure to set `CUDA_DEVICE_ORDER=PCI_BUS_ID` to avoid unexpected behavior.
/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/connections.py:8: RuntimeWarning: Failed to read commit hash:
No module named 'vllm._version'
  from vllm.version import __version__ as VLLM_VERSION
You are using a model of type qwen2_vl to instantiate a model of type . This is not supported for all configurations of models and can yield errors.
INFO 10-17 12:43:30 config.py:887] Defaulting to use mp for distributed inference
INFO 10-17 12:43:30 llm_engine.py:237] Initializing an LLM engine (vdev) with config: model='Qwen/Qwen2-VL-2B-Instruct', speculative_config=None, tokenizer='Qwen/Qwen2-VL-2B-Instruct', skip_tokenizer_init=False, tokenizer_mode=auto, revision=None, override_neuron_config=None, rope_scaling=None, rope_theta=None, tokenizer_revision=None, trust_remote_code=False, dtype=torch.bfloat16, max_seq_len=32768, download_dir=None, load_format=LoadFormat.AUTO, tensor_parallel_size=2, pipeline_parallel_size=1, disable_custom_all_reduce=False, quantization=None, enforce_eager=False, kv_cache_dtype=auto, quantization_param_path=None, device_config=cuda, decoding_config=DecodingConfig(guided_decoding_backend='outlines'), observability_config=ObservabilityConfig(otlp_traces_endpoint=None, collect_model_forward_time=False, collect_model_execute_time=False), seed=0, served_model_name=Qwen/Qwen2-VL-2B-Instruct, use_v2_block_manager=True, num_scheduler_steps=1, chunked_prefill_enabled=False multi_step_stream_outputs=True, enable_prefix_caching=False, use_async_output_proc=True, use_cached_outputs=False, mm_processor_kwargs=None)
WARNING 10-17 12:43:31 multiproc_gpu_executor.py:53] Reducing Torch parallelism from 64 threads to 1 to avoid unnecessary CPU contention. Set OMP_NUM_THREADS in the external environment to tune this value as needed.
INFO 10-17 12:43:31 custom_cache_manager.py:17] Setting Triton cache manager to: vllm.triton_utils.custom_cache_manager:CustomCacheManager
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:32 multiproc_worker_utils.py:216] Worker ready; awaiting tasks
INFO 10-17 12:43:34 utils.py:1008] Found nccl from library libnccl.so.2
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:34 utils.py:1008] Found nccl from library libnccl.so.2
INFO 10-17 12:43:34 pynccl.py:63] vLLM is using nccl==2.20.5
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:34 pynccl.py:63] vLLM is using nccl==2.20.5
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:35 custom_all_reduce_utils.py:242] reading GPU P2P access cache from /home/aryan/.cache/vllm/gpu_p2p_access_cache_for_2,3.json
INFO 10-17 12:43:35 custom_all_reduce_utils.py:242] reading GPU P2P access cache from /home/aryan/.cache/vllm/gpu_p2p_access_cache_for_2,3.json
INFO 10-17 12:43:35 shm_broadcast.py:241] vLLM message queue communication handle: Handle(connect_ip='<IP_ADDRESS>', local_reader_ranks=[1], buffer=<vllm.distributed.device_communicators.shm_broadcast.ShmRingBuffer object at 0x7f6928dde860>, local_subscribe_port=47105, remote_subscribe_port=None)
INFO 10-17 12:43:35 model_runner.py:1060] Starting to load model Qwen/Qwen2-VL-2B-Instruct...
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:35 model_runner.py:1060] Starting to load model Qwen/Qwen2-VL-2B-Instruct...
INFO 10-17 12:43:35 weight_utils.py:243] Using model weights format ['*.safetensors']
(VllmWorkerProcess pid=554434) INFO 10-17 12:43:35 weight_utils.py:243] Using model weights format ['*.safetensors']
[rank0]: Traceback (most recent call last):
[rank0]:   File "/raid/aryan/cogvideox-distillation/video_recaptioning/recaption.py", line 121, in <module>
[rank0]:     fire.Fire(main)
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/fire/core.py", line 135, in Fire
[rank0]:     component_trace = _Fire(component, args, parsed_flag_args, context, name)
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/fire/core.py", line 468, in _Fire
[rank0]:     component, remaining_args = _CallAndUpdateTrace(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/fire/core.py", line 684, in _CallAndUpdateTrace
[rank0]:     component = fn(*varargs, **kwargs)
[rank0]:   File "/raid/aryan/cogvideox-distillation/video_recaptioning/recaption.py", line 89, in main
[rank0]:     vllm_engine, sampling_params = load_model(
[rank0]:   File "/raid/aryan/cogvideox-distillation/video_recaptioning/recaption.py", line 69, in load_model
[rank0]:     vllm_engine = LLM(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/entrypoints/llm.py", line 177, in __init__
[rank0]:     self.llm_engine = LLMEngine.from_engine_args(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/engine/llm_engine.py", line 574, in from_engine_args
[rank0]:     engine = cls(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/engine/llm_engine.py", line 335, in __init__
[rank0]:     self.model_executor = executor_class(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/executor/distributed_gpu_executor.py", line 26, in __init__
[rank0]:     super().__init__(*args, **kwargs)
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/executor/executor_base.py", line 47, in __init__
[rank0]:     self._init_executor()
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/executor/multiproc_gpu_executor.py", line 111, in _init_executor
[rank0]:     self._run_workers("load_model",
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/executor/multiproc_gpu_executor.py", line 192, in _run_workers
[rank0]:     driver_worker_output = driver_worker_method(*args, **kwargs)
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/worker/worker.py", line 183, in load_model
[rank0]:     self.model_runner.load_model()
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/worker/model_runner.py", line 1062, in load_model
[rank0]:     self.model = get_model(model_config=self.model_config,
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/__init__.py", line 19, in get_model
[rank0]:     return loader.load_model(model_config=model_config,
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/loader.py", line 402, in load_model
[rank0]:     model.load_weights(self._get_all_weights(model_config, model))
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/models/qwen2_vl.py", line 1126, in load_weights
[rank0]:     for name, loaded_weight in weights:
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/loader.py", line 377, in _get_all_weights
[rank0]:     yield from self._get_weights_iterator(primary_weights)
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/loader.py", line 336, in _get_weights_iterator
[rank0]:     hf_folder, hf_weights_files, use_safetensors = self._prepare_weights(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/loader.py", line 292, in _prepare_weights
[rank0]:     hf_folder = download_weights_from_hf(
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/vllm/model_executor/model_loader/weight_utils.py", line 246, in download_weights_from_hf
[rank0]:     with get_lock(model_name_or_path, cache_dir):
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/filelock/_api.py", line 297, in __enter__
[rank0]:     self.acquire()
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/filelock/_api.py", line 255, in acquire
[rank0]:     self._acquire()
[rank0]:   File "/raid/aryan/nightly-venv/lib/python3.10/site-packages/filelock/_unix.py", line 39, in _acquire
[rank0]:     fd = os.open(self.lock_file, open_flags, self._context.mode)
[rank0]: PermissionError: [Errno 13] Permission denied: '/tmp/31ada50e7aab2c9a44d651c511a187e63ec6fd0d7ba11408530179c4c8ea1eb2Qwen-Qwen2-VL-2B-Instruct.lock'
INFO 10-17 12:43:35 multiproc_worker_utils.py:121] Killing local vLLM worker processes
Fatal Python error: _enter_buffered_busy: could not acquire lock for <_io.BufferedWriter name='<stdout>'> at interpreter shutdown, possibly due to daemon threads
Python runtime state: finalizing (tstate=0x0000563edffa8600)

Current thread 0x00007f69dc88c740 (most recent call first):
  <no Python frame>

Extension modules: numpy.core._multiarray_umath, numpy.core._multiarray_tests, numpy.linalg._umath_linalg, numpy.fft._pocketfft_internal, numpy.random._common, numpy.random.bit_generator, numpy.random._bounded_integers, numpy.random._mt19937, numpy.random.mtrand, numpy.random._philox, numpy.random._pcg64, numpy.random._sfc64, numpy.random._generator, torch._C, torch._C._fft, torch._C._linalg, torch._C._nested, torch._C._nn, torch._C._sparse, torch._C._special, charset_normalizer.md, requests.packages.charset_normalizer.md, requests.packages.chardet.md, yaml._yaml, markupsafe._speedups, PIL._imaging, psutil._psutil_linux, psutil._psutil_posix, msgspec._core, sentencepiece._sentencepiece, PIL._imagingft, jaxlib.cpu_feature_guard, regex._regex, msgpack._cmsgpack, google.protobuf.pyext._message, setproctitle, uvloop.loop, ray._raylet, multidict._multidict, yarl._quoting_c, aiohttp._helpers, aiohttp._http_writer, aiohttp._http_parser, aiohttp._websocket, frozenlist._frozenlist, zmq.backend.cython._zmq (total: 46)
/home/aryan/.pyenv/versions/3.10.14/lib/python3.10/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '
./launch.sh: line 11: 554077 Aborted                 (core dumped) python recaption.py --root_dir="/raid/aryan/video-dataset-tom-and-jerry" --output_dir="/raid/aryan/video-dataset-tom-and-jerry-recaptioned" --num_devices=2 --max_num_frames=8 --max_tokens=120 --num_data_workers=4 --batch_size=2 --prompt="Please describe the content of this video in as much detail as possible, including the objects, scenery, animals, characters, and camera movements within the video. Do not include '\n' in your response. Please start the description with the video content directly. Please describe the content of the video and the changes that occur, in chronological order." --num_artifact_workers=4

It's a permission denied error, what can I do to sort your permissions? :P

I can lift it either because I don't have sudo. Better to check in infra.

I'm looking through the vLLM docs to see if they have an environment variable I could configure to use different cache dir, but if not then will ask in infra. Thanks!

Seems like adding a download_dir param fixes it with multiple users. I'll add the relevant changes and optional arg to the script: https://github.com/vllm-project/vllm/issues/2675

I will try a few models to see what works best as default. I personally preferred the outputs of MiniCPM a lot, but will also give Qwen 7b a try. Currently, getting descriptions like:
In these frames, we see an animated scene involving a dog character.  The weapon appears to be a sword, and the dog’s expression suggests that they are in the middle of making a move or about to strike. The scene is dynamic, showing the dog in a poised position as they execute the attack or защиты.This is an encoded image sequence of an animated character peacefully hanging in an industrial setting. The character is depicted in a suspended position, seemingly balancing against pipes and metal infrastructure. The environment appears to be confined spaces with industrial fixtures, suggesting a engineering or industrial context.In the given sequence of frames, we see an animated scene featuring two characters associated with a renowned animated series. They are placed in a setting with a brown wall and green floor.

1. At the top of the frame, there is Tom, a character frequently seen in the series, leading in a playful manner. He is dressed in a red dress jacket and surrounded by polka-dotted socks. His head is adorned with a porkpie hat, and his facial expression suggests a cheerful and jovial demeanor.

2. Below him, a character dressed in a blue outfit can be observed.The frames depict a scene of two animated characters sitting on a bench. The character on the left is a cat wearing a cowboy hat and holding a paper bag, while the character on the right is a cow. They appear to be talking or engaged in conversation. The background suggests a makeshift cabin or shed, indicating a rural or desert setting.The scene depicts a close-up of a person sitting in what appears to be a car or a vehicle. The person is seated on a seat, and the background shows a white curtain hanging behind them, possibly indicating that the vehicle is indoors. The overall atmosphere is indoor and cozy, conveying a sense of comfort and relaxation.In the presented sequence of frames, a character wearing white gloves comes into view. The character's white gloves have relaxed and ungrip betting in a peculiar way. The palm-side of the human finger and pointer finger are facing upward with fingers sticking straight out for a distance that seems asymptomatic. Both the hand's back and the fingers' heads are oriented parallel to each other in a high-angle manner, raising the combined formation in a peculiar fashion. The whole posture and positioning of the gloves may signify surprise or surprise-breathing or diahorrea like/unlike roleplay. Other hands purely in casual colourthis set of frames(2,90),(986,613) are quick and energetic.The frames in the video depict an animated scene involving a cat character who appears to be bundles wrapped around timbers or a similar object. The cat is wearing red trousers, white tennis shoes, and seems to be in a festive or playful mood. Surrounding the cat is a similarly dressed character and an ocean scene. The cat holds one of the bundles in its left hand, and it seems to be engaged in a lighthearted task, possibly navigating the bundles to reach a destination or dealing with them as part of a playful activity.a gray cat around a wall, dancing near the sill.The sequence depicts a view from a ship's Wildlife Pool, showcasing a swimming pool surrounded by a diving board with climbing stairs on both sides. The swimming pool has both a shallow end and a deep end, and is flanked by railings that provide an elevated diving ramp for participants. The diving board is adjacent to the pool, and the railing curve along the side of the deep end provides a protective barrier.In the beginning, a cute red ax is visually engaging punches the blue oar in a chilling blue water.Overall, the frames depict a scene of a white boat sailing under a bridge. The scene appears to be calm, with no任何人 attach themselves easilyThis is an animated scene with a curious mouse named Jerry. The mouse is standing in front of a wooden wall, displaying a mix of surprise, confusion, and possibly disbelief. The mouse has large, expressive ears and is wearing a black bow tie, adding a playful element to its expression. The background includes a room with a内阁的 wood and a wooden shelf, which adds to the setting.A cat with a fish tail appears in front of a bathroom shower, pero Suddenly, the cat falls backward and dyes the water behind it light blue.this set of frames.(2,2),(994,990) describes where there is a chair standing by the wall.This set of frames depicts a cartoon scene involving two characters. The character on the left is a anthropomorphic cat named Tom, who is wearing a gray coat and has a frying pan around his neck. He is holding a baseball bat in his right hand. The character on the right is a small brown dog named Jerry, who is wearing a blue coat and has a plastic dog toy in its mouth.

Does not seem to be respecting the 120 token limit set when lauching :thinking:

I will try a few models to see what works best as default.

Take note of the following from https://github.com/a-r-r-o-w/cogvideox-factory/pull/34#issue-2588236287

Not all models listed in https://docs.vllm.ai/en/latest/models/supported_models.html will have the same format for doing video captioning. Change the recaption.py as needed to suit your needs.

Does not seem to be respecting the 120 token limit set when lauching 🤔

That is likely a issue for vllm then.
Configuration-wise, we can experiment but I expected code-related comments as the first set of comments.


undefined method `release' for OS::Mac:Module

Don't know if this is already addressed but the cli told me to report this bug ;-)
brew cask info libreoffice      Error: undefined method release' for OS::Mac:Module
Please report this bug:
https://github.com/caskroom/homebrew-cask/issues
:1:in `'
:in `'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/source/path_base.rb:55:in `eval'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/source/path_base.rb:55:in `rescue in load'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/source/path_base.rb:52:in `load'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/source/tapped.rb:28:in `load'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask.rb:123:in `load'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/cli/info.rb:7:in `block in run'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/cli/info.rb:5:in `each'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/cli/info.rb:5:in `run'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/cli.rb:81:in `run_command'
/usr/local/Cellar/brew-cask/0.44.0/rubylib/cask/cli.rb:121:in `process'
/usr/local/bin/brew-cask.rb:42:in `'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/usr/local/Library/brew.rb:58:in `require?'
/usr/local/Library/brew.rb:141:in `'`

Hi there, you're using an outdated version of homebrew-cask, upgrading via the following should set you straight:
brew update && brew upgrade brew-cask && brew cleanup && brew cask cleanup


consider linking https://github.com/ralphtheninja/open-funding

https://github.com/ralphtheninja/open-funding is more focused ("funding open source projects") so probably it would not be useful to copy programs, but maybe it would be useful to link it?

If that is a good idea - let me know and I will try making a PR.

Hi @matkoniecz, thanks for flagging! I haven't been sure what to do with open source grant programs, because - given my line of work - I'm aware of so many, but I don't want to overwhelm this list (which is not OSS-focused).
I think it'd help to add a section to https://github.com/nayafia/lemonade-stand instead, with links to these resources.
If you want to take a stab at opening a PR over there, it'd mean:

Add a "Related resources" section to the appendix, i.e.:

APPENDIX: [Related resources](#related-resources) // [Contributing to this guide](#contributing-to-this-guide) // [License & attribution](#license-and-attribution)

Add a bit of intro text + links to these guides:

https://github.com/ralphtheninja/open-funding
https://github.com/fredexed/crypto-funding-ops (another guide I find myself linking to)

WDYT? No worries if you don't have time, I'll do it either way, but thought I'd see if you're interested first, since it's your idea 😊

I opened https://github.com/nayafia/lemonade-stand/pull/132 - I think that more inline linking would be better
I still think that linking here entire https://github.com/ralphtheninja/open-funding would be better as it links specific projects (what better matches microgrants) while https://github.com/nayafia/lemonade-stand is far more general

thx! merged that PR.

I still think that linking other lists of (micro)grants like https://github.com/ralphtheninja/open-funding#grants from here would be a good idea


Workspace - An error after completing the connection of the bank account

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Action Performed:

Go to URL https://staging.new.expensify.com/
Login with any account
Go to Settings -> Workspaces -> Open any WS -> Connect bank account -> Connect online with Plaid
On the bank account list select "Chase"
Complete the connection of the bank account

Expected Result:
The bank account is connected without error
Actual Result:
An unexpected error occurred while trying to add bank account. Please try again.
Workaround:
Unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android / native
[x] Android / Chrome
[ ] iOS / native
[x] iOS / Safari
[x] Windows / Chrome
[ ] MacOS / Desktop

Version Number: <IP_ADDRESS>
Reproducible in staging?: Yes
Reproducible in production?: n/a
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/236002430-9dc25ce1-05ca-490d-a143-bed820ea9e45.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)

If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead

[ ] This bug is reproducible using the reproduction steps in the OP. S/O

If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
If the reproduction steps aren't clear and you determine the correct steps, please update the OP.

[ ] This issue is filled out as thoroughly and clearly as possible

Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.

[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Adding Deploy label, because QA team is unable to verify in Production.

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

Adding Deploy label, because QA team is unable to verify in Production.

I think maybe we can start by just looking at logs to see why we are getting this error. What account was used @kbecciv ?

Looks like some kind of auto verify failure (logs). Can you try to retest? Maybe some information was not entered correctly.

Also, can you confirm that this is a test that is normally run and works on staging in the past? Or is this the first time we have discovered this bug while testing? Asking because it looks like entering the information into Plaid didn't work there. But we continued to add an account manually and I'm not seeing a clear reason why we didn't get through the initial Plaid step.

I don't think selecting "Chase" is correct. The one that works is "Wells Fargo".

@MelvinBot Yes, Chase bank, adding TR link https://expensify.testrail.io/index.php?/tests/view/3436330

Maybe sus? https://github.com/Expensify/App/pull/18103

I was able to successfully connect a bank account on staging:

Going to close this out.


Fix for not creating attributes in depending tables

Description
Please describe your pull request:

Why is it necessary? attributes in depending tables are not created
What does it improve? creates attributes in depending tables
Does it have side effects? no

Questions
Answers

BC breaks?
no

Tests pass?
yes

Related tickets?

How to test?
see below

$this->get('shopware_attribute.crud_service')->update(
                's_user_addresses_attributes',
                'abc_test',
                'boolean',
                array(),
                null,
                true
            );

this will create an attribute abc_test in s_user_addresses_attributes but not in s_user_shippingaddress_attributes,
s_user_billingaddress_attributes,
s_order_shippingaddress_attributes,
s_order_billingaddress_attributes
This pull request will fix this, so the attributes are created as expected

Hi,
makes sense, thanks for the PR!

Hey,
you shoud extends the dispatch query in the product export too.
https://github.com/shopware/shopware/blob/5.2/engine/Shopware/Core/sExport.php#L1330
Or the select on the attr table generated there an error.
Alternatively, you can also use a subselect . Then the JOIN does not need.
Heiner


Custom editor group contributions.configuration

Hi vscode
We have multiple extensions as part of an extension pack, I would like to create some settings for these extensions, the main extension should create 1 entry in settings and then any dependencies would add there settings underneath it.
Current behaviour appears to be every extension has to have its own entry in preferences/settings/extensions
thanks

Is this feature request specifically for how the settings show up in the UX Settings editor, or does it also have to do with the way the settings are being configured in the package.json file?


fix: add tests for metric-imperial-converter (infosec)

[x] I have read freeCodeCamp's contribution guidelines.
[x] My pull request has a descriptive title (not a vague title like Update index.md)
[x] My pull request targets the master branch of freeCodeCamp.
[x] None of my changes are plagiarized from another source without proper attribution.
[x] All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
[x] My changes do not use shortened URLs or affiliate links.

Contributes to #37185

@Aadhar3 If you surround the testString value with double quotation marks, then you would not need the two single quotes for the code.

I'm not sure why, but these aren't passing with the example project for me. Were you able to get them to pass with the example project @Aadhar3?

@Aadhar3 do you have a chance to look into Tom's concerns?  We're trying to (belatedly, I realise!) finish reviewing outstanding PRs.

Blocked because of the pinned code freeze issue - The PR can still be fixed up and reviewed, just not merged at the moment.

Limitations removed on QA Certs.

Hey @moT01 , are we going to go ahead with this PR, even though it does not cover all of the test cases?

@Aadhar3 Can you provide an update on the implementation of the requested changes?

Ideally, we would have tests for everything @Sky020. But I think some tests are better than none - so I would vote to add it. The OP here doesn't look all that active recently so we may have to make any adjustments ourselves.

@moT01 Are we going to go ahead and commit the changes ourselves? Or, is this one of the PRs we can let be fixed post-restructure?

@Sky020 I think it is fine if you want to go ahead and push an extra commit for the changes.  Then Tom and I can do the review/test the final changes and approve/merge if everything looks good.

That is not one commit. I can squash that, if need be?

That is not one commit. I can squash that, if need be?

We will squash all the commits at the end anyway, when we merge it, so don't worry about doing it now.

@moT01 I am trying to debug, but the build of the challenges is not working. I keep getting:

fcc-replace-challenge
validate is not a function

In the client

@Sky020 it's possible you need to reinstall things.  General sanity checks: npm run clean && npm ci, cp sample.env .env and npm run seed make sure your install, enviroment and dev user are all up to date.

@ojeytonwilliams I have not been able to get it fixed. I have needed to npm run develop:client each time I make a change.

@moT01 I took your advice to convert it to async...await (just looks better anyway). I changed one test which required 6 dp precision to 5.
If you could let me know about the addition of the code tags. I was unsure whether to leave the ' or not. I did, because it is a string being passed.


Changes relating to responsive column headers

Hi Thomas,
In addition to these commits, I'm also wondering the following:
Could we create a dist folder in the root to house the following files?

data-grid.js (unminified,  but bundled with all plugins, utils, and imports from /data-grid.js)
data-grid.min.js (moved from /data-grid.min.js - minified and bundled with all plugins and dependencies)
data-grid.css (copy of /data-grid.css unminified)
data-grid.min.css (moved from /data-grid.min.css - minified)

Thank you again!

@jackkjhsu that looks great. yes we can create a dist folder, no worries
one important thing, could you apply your changes to the scss file and then simply compile the scss to css ? it seems you make the changes directly in the compiled css file and that's not very convenient.

@jackkjhsu fyi, it's very likely i'm going to make changes in how the responsive mode is handled. It's very likely i'll use a solution like many other data grids where you have a little toggle icon that appears at the start of a row when some rows are hidden due to lack of space in small screens. this will allow to have a more consistent layout.

@jackkjhsu that looks great. yes we can create a dist folder, no worries one important thing, could you apply your changes to the scss file and then simply compile the scss to css ? it seems you make the changes directly in the compiled css file and that's not very convenient.

I usually deal with css directly and not bother with scss in my own work, but I will look into applying these css changes in scss and then do a compile into css.
Thanks

@jackkjhsu fyi, it's very likely i'm going to make changes in how the responsive mode is handled. It's very likely i'll use a solution like many other data grids where you have a little toggle icon that appears at the start of a row when some rows are hidden due to lack of space in small screens. this will allow to have a more consistent layout.

Yeah, that sounds good. Even with the new changes showing the column header THs, I don't think it uses the limited space perfectly.
Thanks

@jackkjhsu mostly good ! i'm going to make a couple of changes but that looks great

@jackkjhsu i made a couple of changes on master you can check if that doesn't break anything for you ?

@jackkjhsu i made a couple of changes on master you can check if that doesn't break anything for you ?

This business of toggling various areas, including the column headers, of the grid based on the grid's properties, i.e. responsive, sort, filter and selectable, quickly get pretty complex to do it properly with just css. The latest changes introduced an issue where the "Column Filters" label is hidden when it needs to be shown. I think it needs to be shown whenever the grid's width < 768 and [responsive][filter="true"] . So I am creating another pull request where I kind of went back to the changes I made previously but with a few tweaks. The css styles at the bottom of scss file is not really pretty but I think it's necessary to cover all different states of the grid without resorting to javascript hacks.
I am also including a new change to fix an issue where the data displayed in the columns is not aligned after the columns are reordered and the grid retrieves the data in server mode.
Thanks Thomas

@jackkjhsu yes i'm not happy with the current logic, it should be simpler. Overall, I think a big refactor is needed anyway :-) ideally I'd like to have each feature clearly split in a distinct class.

@jackkjhsu yes i'm not happy with the current logic, it should be simpler. Overall, I think a big refactor is needed anyway :-) ideally I'd like to have each feature clearly split in a distinct class.

@lekoala, I think it's probably good to do a refactoring after the responsive column header redesign you talked about earlier. Nevertheless, it is functioning for now as it's supposed to, and even the column reordering is working as intended in responsive mode. I will keep the changes coming as needed, since I will be working with the data-grid for the next little while and trying to incorporate it in a working web app. I think that's the best way to weed out any bug it may still have.
Thanks again Thomas.


Update README.md

change folder/wbl to folder.wbl

its all good
On Fri, 16 Aug 2019 at 16:02, josh-asi<EMAIL_ADDRESS>wrote:

Merged #1
https://github.com/josh-asi/qlik-mashup-react-boilerplate/pull/1 into
master.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/josh-asi/qlik-mashup-react-boilerplate/pull/1?email_source=notifications&email_token=AKWQT5V5QK44YFLGKDXEDOTQEYRG5A5CNFSM4IMDZAMKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTCU2OVQ#event-2561255254,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKWQT5WWKW53NVA5D535RDDQEYRG5ANCNFSM4IMDZAMA
.


fix: navigation bar color

This PR resolves MOPLAT-627
Description

moves useAndroidNavigationBarThemeListener to a custom hook and to the Navigation.tsx to get updated correctly
fixes weird behaviors with navigationBar on android behaving weirdly

Demo
https://user-images.githubusercontent.com/21178754/234542492-ff14c70d-5b44-4c0c-b6d7-ca843a98f345.mov

PR Checklist

[x] I have tested my changes on iOS and Android.
[x] I have included screenshots or videos, or I have not changed the UI.
[x] I have added tests, or my changes don't require any.
[x] I added an app state migration, or my changes do not require one.
[x] I have documented any follow-up work that this PR will require, or it does not require any.

To the reviewers 👀

[ ] I would like at least one of the reviewers to run this PR on the simulator or device.

Need help with something? Have a look at our docs, or get in touch with us.

So tricky! tyy


[Snyk] Upgrade html-snapshots from 2.2.0 to 2.3.0

This PR was automatically created by Snyk using the credentials of a real user.Snyk has created this PR to upgrade html-snapshots from 2.2.0 to 2.3.0.
:information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.

The recommended version is 1 version ahead of your current version.
The recommended version was released 2 days ago, on 2023-06-01.

Release notes

Package name: html-snapshots

2.3.0 -<EMAIL_ADDRESS>

2.2.0 - 2023-05-29got 13.0.0

    from html-snapshots GitHub release notes

Commit messages

Package name: html-snapshots

72cf6cc Merge pull request #375 from localnerve/rc-2.3.0
a83d7db @ 2.3.0 -<EMAIL_ADDRESS>da87b6b @ 2.2.0 - update examples

Compare

Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.
For more information:
🧐 View latest project report
🛠 Adjust upgrade PR settings
🔕 Ignore this dependency or unsubscribe from future upgrade PRs

coverage: 99.245%. remained the same when pulling 13c35d499fe0365ed482a80566970c1c97128338 on snyk-upgrade-5973f93154843987ff540eab3d3a417c into 8abc9b1c0257014b626de258a4870202299d111d on master.


We need to clarify the bahaviour of ConverterException

Especially

when will it get thrown?
what information will it contain?
what can the user do with it?

It's not yet clear to me what the intention behind it was.
We need to clarify this in the spec document and the JavaDoc.

Not applicable any more.


Maven support

This is almost done, but not yet ready for merge. I would like to get some feedback before I clean it up, please see the comments in Reviewable.

This change is

Review status: 0 of 18 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

analyzer/src/funTest/assets/projects/synthetic/maven/src/main/java/com/here/ort/maven/App.java, line 1 at r3 (raw file):

package com.here.ort.maven;

Reminder: Either delete test Java files, or move them to the app or lib project.

analyzer/src/funTest/assets/projects/synthetic/maven/src/test/java/com/here/ort/maven/AppTest.java, line 1 at r3 (raw file):

package com.here.ort.maven;

Reminder: Either delete test Java files, or move them to the app or lib project.

analyzer/src/main/kotlin/PackageManager.kt, line 120 at r3 (raw file):

     * Optional preparation step for dependency resolution, like checking for prerequisites.
     */
    protected open fun prepareResolution(definitionFiles: List<File>) {

This was required because I wanted to share the container and session between this method and the calls to resolveDependency below. Please see my comment in Maven.kt below.

analyzer/src/main/kotlin/managers/Maven.kt, line 75 at r3 (raw file):

    private val container = createContainer()
    private val repositorySystemSession = createRepositorySystemSession()
    private val projectsByIdentifier = mutableMapOf<String, ProjectBuildingResult>()

I think the singleton approach is at it's limit here. The three objects above should be reused within one call of PackageManager.resolveDependencies but not more. Currently they are for example consistent across all test in MavenTest which is not desired. Even if I could share the objects between prepareResolution() and resolveDependencies() I would have to pass them around to almost all methods which would make the code harder to read (I had it like this before).
My idea is that we make the PackageManager classes factories that create the actual implementations, then we could use class fields without side effects.

Comments from Reviewable

Reviewed 2 of 17 files at r4, 17 of 18 files at r5.
Review status: 13 of 20 files reviewed at latest revision, 42 unresolved discussions.

analyzer/src/funTest/assets/projects/synthetic/maven/src/main/java/com/here/ort/maven/App.java, line 1 at r3 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
Reminder: Either delete test Java files, or move them to the app or lib project.

Looks like you've addressed this comment of yours as this file is inside a new synthetic project, correct?

analyzer/src/funTest/assets/projects/synthetic/maven/src/main/java/com/here/ort/maven/App.java, line 7 at r5 (raw file):

 *
 */
public class App

Nit: Trailing space.

analyzer/src/funTest/assets/projects/synthetic/maven/src/main/java/com/here/ort/maven/App.java, line 9 at r5 (raw file):

public class App
{
    public static void main( String[] args )

Nit: Non-standard formatting around parentheses (throughout the file).

analyzer/src/funTest/assets/projects/synthetic/maven/src/test/java/com/here/ort/maven/AppTest.java, line 10 at r5 (raw file):

 * Unit test for simple App.
 */
public class AppTest

Nit: Trailing space.

analyzer/src/funTest/assets/projects/synthetic/maven/src/test/java/com/here/ort/maven/AppTest.java, line 18 at r5 (raw file):

     * @param testName name of the test case
     */
    public AppTest( String testName )

Nit: Non-standard formatting around parentheses (throughout the file).

analyzer/src/funTest/kotlin/MavenTest.kt, line 25 at r5 (raw file):

import com.here.ort.util.log
import com.here.ort.util.yamlMapper
import io.kotlintest.TestCaseContext

Nit: Move this to the block below.

analyzer/src/funTest/kotlin/MavenTest.kt, line 35 at r5 (raw file):

    private val syntheticProjectDir = File("src/funTest/assets/projects/synthetic/maven")

    @Suppress("CatchException")

Why? We're not catching anything below.

analyzer/src/funTest/kotlin/MavenTest.kt, line 36 at r5 (raw file):

    @Suppress("CatchException")
    override fun interceptTestCase(context: TestCaseContext, test: () -> Unit) {

Is this whole function left-over debug code?

analyzer/src/funTest/kotlin/MavenTest.kt, line 43 at r5 (raw file):

    init {
        "jgnash parent dependencies are detected correctly" {
            // TODO: test not only root project of JGnash.

Is this still valid as you have a test for not only testing the root below?

analyzer/src/funTest/kotlin/MavenTest.kt, line 54 at r5 (raw file):

        "jgnash-core dependencies are detected correctly" {
            // TODO: test not only root project of JGnash.

Same here, is this still valid? I believe not testing only the root project is exactly what this test does, or?

analyzer/src/funTest/kotlin/MavenTest.kt, line 60 at r5 (raw file):

            val expectedResult = File(projectDir.parentFile, "jgnash-core-expected-output.yml").readText()

            val result = Maven.resolveDependencies(projectDir, listOf(pomFileCore, pomFileResources))[pomFileCore]

Why do you pass in pomFileResources, but then only compare the results for pomFileCore? I guess you have to pass in resources as core depends on it, but shouldn't you then also compare resources results? Or are resources results included in the core results exactly because of that dependency? I guess that deserves a code comment.

analyzer/src/funTest/kotlin/MavenTest.kt, line 79 at r5 (raw file):

            val expectedResult = File(syntheticProjectDir.parentFile, "project-maven-expected-output-app.yml").readText()

            val result = Maven.resolveDependencies(syntheticProjectDir, listOf(pomFileApp, pomFileLib))[pomFileApp]

Similar case to above, why lib results are not compared? If they are included in the app results, please explain that in a code comment.

analyzer/src/main/kotlin/Main.kt, line 154 at r5 (raw file):

        jc.programName = "analyzer"

        log.level = ch.qos.logback.classic.Level.WARN

Left-over debug code? In any case, the default level should already be WARN, see util/src/main/resources/logback.xml.

analyzer/src/main/kotlin/MavenLogger.kt, line 23 at r5 (raw file):

import ch.qos.logback.classic.Level
import com.here.ort.util.log

Nit: Missing blank lines before and after this line.

analyzer/src/main/kotlin/MavenLogger.kt, line 27 at r5 (raw file):

: Int {

You might want to just use =.

analyzer/src/main/kotlin/MavenLogger.kt, line 40 at r5 (raw file):

}

class MavenLogger(level: Level) : AbstractLogger(toPlexusLoggerLevel(level), log.name) {

Please add some minimum high-level class docs explaining why this class is necessary. (Because we use logback, and Maven uses plexus, and we want to Maven to adhere to our set logging level.)

analyzer/src/main/kotlin/PackageManager.kt, line 120 at r3 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
This was required because I wanted to share the container and session between this method and the calls to resolveDependency below. Please see my comment in Maven.kt below.

I'm fine with that change anyway. It's absolutely thinkable that the preparation also for other package managers already needs to know about the definition files (e.g. in order to filter them, in case of subprojects, or so).

analyzer/src/main/kotlin/managers/Maven.kt, line 75 at r3 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
I think the singleton approach is at it's limit here. The three objects above should be reused within one call of PackageManager.resolveDependencies but not more. Currently they are for example consistent across all test in MavenTest which is not desired. Even if I could share the objects between prepareResolution() and resolveDependencies() I would have to pass them around to almost all methods which would make the code harder to read (I had it like this before).
My idea is that we make the PackageManager classes factories that create the actual implementations, then we could use class fields without side effects.

Using factories sounds good to me. Is this something you'd like to do after this has been merged, or as part of this PR?

analyzer/src/main/kotlin/managers/Maven.kt, line 23 at r5 (raw file):

import ch.frankel.slf4k.*
import com.here.ort.analyzer.MavenLogger

Nit: Move this line to the block below.

analyzer/src/main/kotlin/managers/Maven.kt, line 75 at r5 (raw file):

        listOf("pom.xml")
) {
    private val NON_DELIVERED_SCOPES = setOf("test")

Why is this in all-CAPS whereas the below private val are not? Also, we should probably make clearer from the variable name that this is only what we assume by default (and whats get written into the YAML file), but it can be overridden later (by editing the YAML file). How about ASSUME_NON_DELIVERED_SCOPES?

analyzer/src/main/kotlin/managers/Maven.kt, line 89 at r5 (raw file):

        val projectBuildingRequest = createProjectBuildingRequest(false)
        val projectBuildingResults = projectBuilder.build(definitionFiles, false, projectBuildingRequest)
        projectBuildingResults.forEach { projectBuildingResult ->

Nit: Blank line before this one.

analyzer/src/main/kotlin/managers/Maven.kt, line 92 at r5 (raw file):

            val project = projectBuildingResult.project
            val identifier = "${project.groupId}:${project.artifactId}:${project.version}"
            println("pomFile: ${projectBuildingResult.pomFile.absolutePath}")

Should we use logging (info level?) here instead?

analyzer/src/main/kotlin/managers/Maven.kt, line 125 at r5 (raw file):

                declaredLicenses = mavenProject.licenses.map { it.name }.toSortedSet(),
                aliases = emptyList(),
                vcsPath = "",

Nit: Make the order of arguments match the new YAML order, e.g. vcsPath comes after vcsRevision.

analyzer/src/main/kotlin/managers/Maven.kt, line 144 at r5 (raw file):

    private fun createContainer(): PlexusContainer {
        val configuration = DefaultContainerConfiguration()

How about using an apply {}block to get rid of the configuration. prefixes below?

analyzer/src/main/kotlin/managers/Maven.kt, line 160 at r5 (raw file):

    private fun createProjectBuildingRequest(resolveDependencies: Boolean): ProjectBuildingRequest {
        val projectBuildingRequest = DefaultProjectBuildingRequest()

You may want to use apply {} again, then this could even become a expression body = function.

analyzer/src/main/kotlin/managers/Maven.kt, line 183 at r5 (raw file):

    private fun parseDependency(node: DependencyNode, packages: MutableMap<String, Package>): PackageReference {
        val pkg = packages.getOrPut("${node.artifact.groupId}:${node.artifact.artifactId}:${node.artifact.version}") {

Also use
val identifier = node.artifact.let { "${it.groupId}:${it.artifactId}:${it.version}" }

here?

analyzer/src/main/kotlin/managers/Maven.kt, line 189 at r5 (raw file):

        val dependencies = sortedSetOf<PackageReference>()

        node.children.forEach { child ->

Maybe instead:
dependencies.addAll(node.children.map { parseDependency(it, packages) })

analyzer/src/main/kotlin/managers/Maven.kt, line 199 at r5 (raw file):

        val mavenRepositorySystem = container.lookup(MavenRepositorySystem::class.java, "default")
        val projectBuilder = container.lookup(ProjectBuilder::class.java, "default")
        val projectBuildingRequest = createProjectBuildingRequest(true)

Use apply {} again to combine with below line?

analyzer/src/main/kotlin/managers/Maven.kt, line 205 at r5 (raw file):

        val cachedProject = projectsByIdentifier[identifier]

        val mavenProject = if (cachedProject != null) {

Not sure if that's actually more readable, but you could use instead:
    val mavenProject = cachedProject?.project ?:
            projectBuilder.let {
                val artifact = node.artifact.let {
                    mavenRepositorySystem.createArtifact(it.groupId, it.artifactId, it.version, "", "pom")
                }
                it.build(artifact, projectBuildingRequest).project
            }

analyzer/src/main/kotlin/managers/Maven.kt, line 225 at r5 (raw file):

                hash = "", // TODO: Get hash from local metadata?
                hashAlgorithm = "",
                vcsPath = "",

Nit: Move vcsPath after vcsRevision again.

analyzer/src/main/kotlin/managers/Maven.kt, line 233 at r5 (raw file):

    private fun parseVcsProvider(mavenProject: MavenProject): String {
        mavenProject.scm?.let {

mavenProject.scm?.connection?.let {}

analyzer/src/main/kotlin/managers/Maven.kt, line 236 at r5 (raw file):

            it.connection?.let {
                val split = it.split(":")
                if (split.size > 1) {

Maybe add another sanity check that && split[0] == "scm"?

analyzer/src/main/kotlin/managers/Maven.kt, line 245 at r5 (raw file):

    private fun parseVcsRevision(mavenProject: MavenProject): String {
        mavenProject.scm?.let {

Expression body function with
= mavenProject.scm?.tag ?: ""

analyzer/src/main/kotlin/managers/Maven.kt, line 253 at r5 (raw file):

    private fun parseVcsUrl(mavenProject: MavenProject): String {
        mavenProject.scm?.let {
            it.url?.let { return it }

Nope. We should never return the <url> tag here as that the URL to "A publicly browsable repository. For example, via ViewCVS" [1], i.e. a web UI.
[1] https://maven.apache.org/pom.html#SCM

analyzer/src/main/kotlin/managers/Maven.kt, line 255 at r5 (raw file):

            it.url?.let { return it }
            it.connection?.let {
                val split = it.split(".")

This should split by :, or?

analyzer/src/main/kotlin/managers/Maven.kt, line 256 at r5 (raw file):

            it.connection?.let {
                val split = it.split(".")
                if (split.size > 2) {

Similar sanity check for split[0] like above should be added, similar let simplifications probably apply.

analyzer/src/main/kotlin/managers/Maven.kt, line 267 at r5 (raw file):

     * A wrapper for the [LocalRepositoryManager] used in [repositorySystemSession] that pretends that the POM files of
     * the currently analyzed project are available in the local repository. Without it the resolution of transitive
     * dependencies of project dependencies would not work without installing the project in the local repository first.

Cool stuff!

analyzer/src/main/kotlin/managers/Maven.kt, line 287 at r5 (raw file):

                val file = File(it.pomFile.absolutePath)

                val result = LocalArtifactResult(request)

Simplify with apply {}?

analyzer/src/main/kotlin/managers/Maven.kt, line 293 at r5 (raw file):

                return result
            }
            return localRepositoryManager.find(session, request)

Nit: Blank line before this one.

analyzer/src/main/kotlin/managers/Maven.kt, line 303 at r5 (raw file):

            log.debug { "Request path for local artifact: $artifact" }
            val identifier = "${artifact.groupId}:${artifact.artifactId}:${artifact.version}"
            log.debug { "Searching for identifier $identifier in cached projects ${projectsByIdentifier.keys}" }

Nit: Blank line before this one.

analyzer/src/main/kotlin/managers/Maven.kt, line 308 at r5 (raw file):

                return it.pomFile.absolutePath
            }
            return localRepositoryManager.getPathForLocalArtifact(artifact)

Nit: Blank line before this one.

Comments from Reviewable

Review status: 9 of 17 files reviewed at latest revision, 42 unresolved discussions.

analyzer/src/funTest/kotlin/MavenTest.kt, line 25 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Move this to the block below.

Done.

analyzer/src/funTest/kotlin/MavenTest.kt, line 35 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Why? We're not catching anything below.

Done. Copy-paste...

analyzer/src/funTest/kotlin/MavenTest.kt, line 36 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Is this whole function left-over debug code?

No, I have to set log.level to something not null for MavenLogger to be initialized correctly, I thought debug is a good default for tests in case something fails.

analyzer/src/funTest/kotlin/MavenTest.kt, line 43 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Is this still valid as you have a test for not only testing the root below?

Done.

analyzer/src/funTest/kotlin/MavenTest.kt, line 54 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Same here, is this still valid? I believe not testing only the root project is exactly what this test does, or?

Done.

analyzer/src/funTest/kotlin/MavenTest.kt, line 60 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Why do you pass in pomFileResources, but then only compare the results for pomFileCore? I guess you have to pass in resources as core depends on it, but shouldn't you then also compare resources results? Or are resources results included in the core results exactly because of that dependency? I guess that deserves a code comment.

Done.

analyzer/src/funTest/kotlin/MavenTest.kt, line 79 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Similar case to above, why lib results are not compared? If they are included in the app results, please explain that in a code comment.

Done.

analyzer/src/main/kotlin/MavenLogger.kt, line 23 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Missing blank lines before and after this line.

Done.

analyzer/src/main/kotlin/MavenLogger.kt, line 27 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…

: Int {

You might want to just use =.

Done.

analyzer/src/main/kotlin/MavenLogger.kt, line 40 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Please add some minimum high-level class docs explaining why this class is necessary. (Because we use logback, and Maven uses plexus, and we want to Maven to adhere to our set logging level.)

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 75 at r3 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Using factories sounds good to me. Is this something you'd like to do after this has been merged, or as part of this PR?

I will do it in a separate PR.

analyzer/src/main/kotlin/managers/Maven.kt, line 23 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Move this line to the block below.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 75 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Why is this in all-CAPS whereas the below private val are not? Also, we should probably make clearer from the variable name that this is only what we assume by default (and whats get written into the YAML file), but it can be overridden later (by editing the YAML file). How about ASSUME_NON_DELIVERED_SCOPES?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 89 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Blank line before this one.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 92 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Should we use logging (info level?) here instead?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 125 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Make the order of arguments match the new YAML order, e.g. vcsPath comes after vcsRevision.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 144 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
How about using an apply {}block to get rid of the configuration. prefixes below?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 160 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
You may want to use apply {} again, then this could even become a expression body = function.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 183 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Also use
val identifier = node.artifact.let { "${it.groupId}:${it.artifactId}:${it.version}" }

here?

Good idea, I added an extension function to reduce duplication.

analyzer/src/main/kotlin/managers/Maven.kt, line 189 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Maybe instead:
dependencies.addAll(node.children.map { parseDependency(it, packages) })

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 205 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Not sure if that's actually more readable, but you could use instead:
    val mavenProject = cachedProject?.project ?:
            projectBuilder.let {
                val artifact = node.artifact.let {
                    mavenRepositorySystem.createArtifact(it.groupId, it.artifactId, it.version, "", "pom")
                }
                it.build(artifact, projectBuildingRequest).project
            }

I have put it directly in node.artifact.let to have one let less.

analyzer/src/main/kotlin/managers/Maven.kt, line 225 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Move vcsPath after vcsRevision again.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 233 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
mavenProject.scm?.connection?.let {}

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 236 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Maybe add another sanity check that && split[0] == "scm"?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 245 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body function with
= mavenProject.scm?.tag ?: ""

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 253 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nope. We should never return the <url> tag here as that the URL to "A publicly browsable repository. For example, via ViewCVS" [1], i.e. a web UI.
[1] https://maven.apache.org/pom.html#SCM

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 255 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
This should split by :, or?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 256 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Similar sanity check for split[0] like above should be added, similar let simplifications probably apply.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 287 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Simplify with apply {}?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 293 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Blank line before this one.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 303 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Blank line before this one.

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 308 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Blank line before this one.

Done.

analyzer/src/funTest/assets/projects/synthetic/maven/src/main/java/com/here/ort/maven/App.java, line 1 at r3 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Looks like you've addressed this comment of yours as this file is inside a new synthetic project, correct?

I will remove these files in the next revision, they are not required for the test. They were generated by the Maven archetype.

analyzer/src/main/kotlin/Main.kt, line 154 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Left-over debug code? In any case, the default level should already be WARN, see util/src/main/resources/logback.xml.

This did not work for some reason. When the Plexus container is loading the MavenLogger class log.level was still null, setting it programmatically fixed the issue. But I just found out that the level is set in the parent logger of log, so using log.effectiveLevel works.

Comments from Reviewable

Reviewed 5 of 10 files at r6.
Review status: 9 of 17 files reviewed at latest revision, 57 unresolved discussions.

analyzer/src/funTest/kotlin/MavenTest.kt, line 36 at r5 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
No, I have to set log.level to something not null for MavenLogger to be initialized correctly, I thought debug is a good default for tests in case something fails.

I guess we also have two more options: Add a logback.xml to funTest resource to initialize the log level, or make toPlexusLoggerLevel() handle nullable log levels. If we stick to the current solution, I believe it deserves a code comment (to explain why is log.level initially null here, but not in the main apps). What option to you like best?
EDIT: Wait a second, is this now needed at all anymore as you're using log.effectiveLevel in Maven.kt?

analyzer/src/funTest/kotlin/MavenTest.kt, line 65 at r6 (raw file):

            // jgnash-core depends on jgnash-resources, so we also have to pass the pom.xml of jgnash-resources to
            // resolveDependencies so that it is available in the Maven.projectsByIdentifier cache. Otherwise resolution
            // of transitive dependencies would not work.

Ok, that explains why we nee to pass it, but not why we only look at the AnalyzerResult of pomFileCore.

analyzer/src/funTest/kotlin/MavenTest.kt, line 76 at r6 (raw file):

                    .readText()
                    .replaceFirst("vcs_url: \"\"", "vcs_url: \"$vcsUrl\"")
                    .replaceFirst("vcs_revision: \"\"", "vcs_revision: \"$vcsRevision\"")

Extract this to a patchExpectedResult() function like in NpmTest.kt?

analyzer/src/main/kotlin/managers/Maven.kt, line 97 at r6 (raw file):

            val project = projectBuildingResult.project
            val identifier = "${project.groupId}:${project.artifactId}:${project.version}"
            log.debug("Building model for '${projectBuildingResult.pomFile.absolutePath}'.")

Please always use {} syntax for logging. Also blank lines before and after to make this stand out?

analyzer/src/main/kotlin/managers/Maven.kt, line 159 at r6 (raw file):

            loggerManager = object : BaseLoggerManager() {
                override fun createLogger(name: String): Logger {
                    return MavenLogger(log.effectiveLevel)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 235 at r6 (raw file):

    private fun parseVcsInfo(mavenProject: MavenProject): VcsInfo {
        return VcsInfo(parseVcsProvider(mavenProject), parseVcsUrl(mavenProject), parseVcsRevision(mavenProject))

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 247 at r6 (raw file):

    }

    private fun parseVcsRevision(mavenProject: MavenProject) = mavenProject.scm?.tag ?: ""

Nit: Sort this in after parseVcsUrl() as that's the field / usage order?

analyzer/src/main/kotlin/managers/Maven.kt, line 267 at r6 (raw file):

        override fun add(session: RepositorySystemSession, request: LocalArtifactRegistration) {
            return localRepositoryManager.add(session, request)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 271 at r6 (raw file):

        override fun add(session: RepositorySystemSession, request: LocalMetadataRegistration) {
            return localRepositoryManager.add(session, request)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 291 at r6 (raw file):

        override fun find(session: RepositorySystemSession, request: LocalMetadataRequest): LocalMetadataResult {
            return localRepositoryManager.find(session, request)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 306 at r6 (raw file):

        override fun getPathForLocalMetadata(metadata: Metadata): String {
            return localRepositoryManager.getPathForLocalMetadata(metadata)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 311 at r6 (raw file):

        override fun getPathForRemoteArtifact(artifact: Artifact, repository: RemoteRepository, context: String)
                : String {
            return localRepositoryManager.getPathForRemoteArtifact(artifact, repository, context)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 316 at r6 (raw file):

        override fun getPathForRemoteMetadata(metadata: Metadata, repository: RemoteRepository, context: String)
                : String {
            return localRepositoryManager.getPathForRemoteMetadata(metadata, repository, context)

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 320 at r6 (raw file):

        override fun getRepository(): LocalRepository {
            return localRepositoryManager.repository

Expression body = syntax?

analyzer/src/main/kotlin/managers/Maven.kt, line 324 at r6 (raw file):

    }

    private data class VcsInfo(val provider: String, val url: String, val revision: String) {

I'm not sure I'm happy to have this here. It feels like something that should go as an Info inner class to VersionControlSystem.kt or similar, what do you think?

analyzer/src/main/kotlin/managers/Maven.kt, line 326 at r6 (raw file):

    private data class VcsInfo(val provider: String, val url: String, val revision: String) {
        fun isEmpty(): Boolean {
            return provider.isEmpty() && url.isEmpty() && revision.isEmpty()

Expression body = syntax?

Comments from Reviewable

Review status: 9 of 17 files reviewed at latest revision, 57 unresolved discussions.

analyzer/src/funTest/kotlin/MavenTest.kt, line 36 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
I guess we also have two more options: Add a logback.xml to funTest resource to initialize the log level, or make toPlexusLoggerLevel() handle nullable log levels. If we stick to the current solution, I believe it deserves a code comment (to explain why is log.level initially null here, but not in the main apps). What option to you like best?
EDIT: Wait a second, is this now needed at all anymore as you're using log.effectiveLevel in Maven.kt?

No, I removed it in the last patch set.

analyzer/src/funTest/kotlin/MavenTest.kt, line 65 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Ok, that explains why we nee to pass it, but not why we only look at the AnalyzerResult of pomFileCore.

But isn't it clear from the test name that we are only checking jgnash-core dependencies?

analyzer/src/funTest/kotlin/MavenTest.kt, line 76 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Extract this to a patchExpectedResult() function like in NpmTest.kt?

God point, the same needs to be done for GradleTest then, I'll do it in a separate commit later.

analyzer/src/main/kotlin/managers/Maven.kt, line 199 at r5 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Use apply {} again to combine with below line?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 97 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Please always use {} syntax for logging. Also blank lines before and after to make this stand out?

I removed the log statement because it was not true anyway, the model was already built. I will go through the class and at more logs in a separate commit.

analyzer/src/main/kotlin/managers/Maven.kt, line 159 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 235 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 247 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Nit: Sort this in after parseVcsUrl() as that's the field / usage order?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 267 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 271 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 306 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 311 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 316 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 320 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

analyzer/src/main/kotlin/managers/Maven.kt, line 324 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
I'm not sure I'm happy to have this here. It feels like something that should go as an Info inner class to VersionControlSystem.kt or similar, what do you think?

Currently it's only a helper for this class, putting it in the model would make it public API which it isn't , at least now. Maybe wait until we have another place to use it and then think about it again?

analyzer/src/main/kotlin/managers/Maven.kt, line 326 at r6 (raw file):
Previously, sschuberth (Sebastian Schuberth) wrote…
Expression body = syntax?

Done.

Comments from Reviewable

Review status: 9 of 17 files reviewed at latest revision, 56 unresolved discussions, some commit checks failed.

analyzer/src/funTest/kotlin/MavenTest.kt, line 65 at r6 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
But isn't it clear from the test name that we are only checking jgnash-core dependencies?

For me, not clear enough :-) The real point here is that the AnalyzerResult for pomFileResources does not need to be compared separately as it's already contained in the AnalyzerResult for pomFileCore. But I can reword that later according to my taste ;-)

analyzer/src/main/kotlin/managers/Maven.kt, line 324 at r6 (raw file):
Previously, mnonnenmacher (Martin Nonnenmacher) wrote…
Currently it's only a helper for this class, putting it in the model would make it public API which it isn't , at least now. Maybe wait until we have another place to use it and then think about it again?

Agreed.

Comments from Reviewable

@mnonnenmacher Could you please fix the detekt code smells so CI passes? I'll do my (hopefully final) review then.

@mnonnenmacher I'll do the change on your behalf.

Reviewed 1 of 3 files at r2, 15 of 17 files at r4.
Review status: 9 of 17 files reviewed at latest revision, 56 unresolved discussions.

Comments from Reviewable