Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LPS-96625 Not needed #74419

Closed
wants to merge 6 commits into from
Closed

Conversation

4lejandrito
Copy link

No description provided.

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering "ci:test:sf" and "ci:test:relevant" for this pull to run Source Formatter and relevant tests.

Comment "ci:test" to run the full PR Tester for this pull.

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutes 33 seconds 18 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: a4a977fe17bdca67073eabfb71de88b8092d593b

Sender Branch:

Branch Name: pr-277
Branch GIT ID: d60a9a6e7475c9e0e38e806bd5a6d343364c0e8a

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:relevant - 28 out of 32 jobs passed in 1 hour 25 minutes 11 seconds 362 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 73fa9b927147e5a547a2cce44dd1b46ae9ab390d

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 1ad4a3697d0bb3ffaa9582b450832f14fda9faa9

28 out of 32 jobs PASSED
28 Successful Jobs:
For more details click here.

Failures unique to this pull:


Failures in common with acceptance upstream results at a4a977f:

@AliciaGarciaGarcia
Copy link

Just started reviewing :)

@brianchandotcom
Copy link
Owner

Merged. Thank you.
View total diff: 82d9173...79e7216

@brianchandotcom
Copy link
Owner

Merged and pushed upstream..

@4lejandrito there sure is a lot of duplicate code in here? I wish we could more easily add this to each module without duplicating so much code.

@JorgeFerrer do you mind taking a quick look at how we're doing auto tagging? something seems off.

@JorgeFerrer
Copy link

JorgeFerrer commented Jun 13, 2019 via email

@JorgeFerrer
Copy link

Hey @brianchandotcom ,
I went over this today with @4lejandrito and we both agreed that there is too much duplication. The root cause is that we have to account for all the combination of values of two independent variables: the type of service (GCloud, NLP, ...) and the type of asset (Blog Entry, File Entry, Journal Article). That means that when a new value for any of these variables appears, we have to create N classes (one for each of the values of the other variable).

How do we solve this? We need to change the implementation a bit so that we avoid having to create a class for each point in the matrix. We started writing the solution and have it almost working. The keys are:

  1. We will have just one class to implement each type of service. It will be tied to AssetEntry so that the companyId can obtained from it.
  2. We will have just one class for each asset type, which is able to extract the text out of it.

We can then have code in one single place which creates all the combination of the two.

We still have one pending challenge though: the configuration. We are currently providing users with an option to enable or disable all possible combinations of the matrix and we want to continue providing that option. To do that, we need to do some improvements to the configuration framework so that it can be more dynamic and show all available options without needing a specific Configuration interface for them. We believe this shouldn't be too hard and Alejandro is going to do some research to find out how to do it.

Makes sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants