search

thoth-station / storages

Storage and database adapters for project Thoth
https://thoth-station.github.io/
GNU General Public License v3.0
14 stars 16 forks source link

Fix package_license metadata attribute error #2705

Closed mayaCostantini closed 2 years ago

mayaCostantini commented 2 years ago

Related Issues and Dependencies

Fixes https://github.com/thoth-station/storages/issues/2703

This introduces a breaking change

This should yield a new module release

This Pull Request implements

Handle the case when the license metadata is undetected during document sync job.

mayaCostantini commented 2 years ago

/assign @harshad16

sesheta commented 2 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: VannTen Once this PR has been reviewed and has the lgtm label, please ask for approval from harshad16 by writing /assign @harshad16 in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/thoth-station/storages/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
harshad16 commented 2 years ago

From the look of the license-solver the license bit should be dict https://github.com/thoth-station/license-solver/blob/28c29dbf560c7165fd67419a3abfe3b53a1c6f1a/thoth/license_solver/package.py#L57 there was issue in default value sent in case of uncatched license. There is already a pr for fix. https://github.com/thoth-station/license-solver/pull/67

I think that would fix the issue, and we would have to catch this in storages.

VannTen commented 2 years ago

On Thu, Sep 15, 2022 at 06:00:33AM -0700, Harshad Reddy Nalla wrote:

From the look of the license-solver the license bit should be dict https://github.com/thoth-station/license-solver/blob/28c29dbf560c7165fd67419a3abfe3b53a1c6f1a/thoth/license_solver/package.py#L57 there was issue in default value sent in case of uncatched license. There is already a pr for fix. https://github.com/thoth-station/license-solver/pull/67

Hum. Reading that code, maybe license should be an Optional[Dict[str, str]] and replace UNDETECTED with None ? Unless something else can be there in place of UNDETECTED ?

harshad16 commented 2 years ago

Hum. Reading that code, maybe license should be an Optional[Dict[str, str]] and replace UNDETECTED with None ? Unless something else can be there in place of UNDETECTED ?

maybe it , seems like the code of license solver needs more of a closer look. As of now, we can follow the pattern of existing one i feel and explore more going forward.

in any case, i feel we might not have to do the type checking ? or do you feel it just better to check as there is no much effect on runtime?

VannTen commented 2 years ago

On Thu, Sep 15, 2022 at 08:11:24AM -0700, Harshad Reddy Nalla wrote:

in any case, i feel we might not have to do the type checking ? or do you feel it just better to check as there is no much effect on runtime?

I'm not the person to ask ^^. I'm totally sold on strong static type-checking, so I'll always feels more type-correctness is good to have. That's a time allocation problem, though (for example the whole graph module in storages is a bit much) ; so if we solve the problem at hand, that could probably wait.

no much effect on runtime

Type hints are discarded at runtime (at parsing time I guess) so makes that virtually no effect.

Don't know why github keeps dropping my email replies :/

VannTen commented 2 years ago

in any case, i feel we might not have to do the type checking ? or do you feel it just better to check as there is no much effect on runtime?

I'm not the person to ask ^^. I'm totally sold on strong static type-checking, so I'll always feels more type-correctness is good to have. That's a time allocation problem, though (for example the whole graph module in storages is a bit much) ; so if we solve the problem at hand, that could probably wait.

no much effect on runtime

Type hints are discarded at runtime (at parsing time I guess) so makes that virtually no effect.

harshad16 commented 2 years ago

This might need more static type-checking. for now, the already include changes are keeping this in place. we should work on license solver to get the default value and then include the type-checking. Closing this for now. lets address this in future, if we face same issue. thanks for all the discussions and work.