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

2390 #3090

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

2390 #3090

wants to merge 4 commits into from

Conversation

l3r8yJ
Copy link
Contributor

@l3r8yJ l3r8yJ commented Apr 14, 2024

@maxonfjvipon @volodya-lombrozo take a look, please

closes #2390


PR-Codex overview

This PR enhances TranspileMojo by implementing a mechanism for distinguishing "internal" and "external" classes, inspired by Gradle's implementation and api concepts.

Detailed summary

  • Improved handling of internal and external classes
  • Added logic to separate classes into different directories based on package and version
  • Introduced the concept of implementation and api for dependencies

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@l3r8yJ
Copy link
Contributor Author

l3r8yJ commented Apr 15, 2024

@maxonfjvipon @volodya-lombrozo reminder

Copy link
Member

@volodya-lombrozo volodya-lombrozo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l3r8yJ Thank you for the contribution! Could you please make this puzzle more understandable? Imagine newcomers joining project: they should understand what's going on and what they have to do to implement this puzzle by reading the puzzle description.

* - implementation – just includes the dependency for inner usage
* - api – allows to users of API to
* use dependency which was added to project via `api` keyword
* <p/><a href="https://shorturl.at/abns4">More about api and implementation here</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l3r8yJ Does this link actually lead somewhere? I can't open it.

* multiple threads and check that the method works correctly without exceptions.
* We can apply the same approach as mentioned in that post:
* <a href="https://www.yegor256.com/2018/03/27/how-to-test-thread-safety.html">Post</a>
* @todo #2375:90min. Implement mechanism for "inner" and "outer" classes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l3r8yJ To be honest, I didn't clearly understand what you want to do in this puzzle.

First of all. Do you want to "get rid of the TranspileMojo#cleanUpClasses method?" If it is so, it would be nice to put this phrase as a header of the puzzle:

@todo #2375:90min. Get rid of the TranspileMojo#cleanUpClasses method.

Second, it would be nice to answer the "why?" question. Why do we need to do it at all?

@todo #2375:90min. Get rid of the TranspileMojo#cleanUpClasses method.
  We need to remove it because ...

Third, you can suggest some ideas about the possible solutions of the problem you already mentioned.

@todo #2375:90min. Get rid of the TranspileMojo#cleanUpClasses method.
  We need to remove it because ...
  To do so, we can...

However, these are only my suggestions, and you can do it as you want. But please, make this puzzle a bit more understandable.

BTW, I didn't clearly understand what "inner" and "outer" classes mean.

@l3r8yJ
Copy link
Contributor Author

l3r8yJ commented Apr 15, 2024

@volodya-lombrozo I made some clarifications based on your review. Also for better understanding of the term dirty classes, can you explain what means dirty in this context?

* multiple threads and check that the method works correctly without exceptions.
* We can apply the same approach as mentioned in that post:
* <a href="https://www.yegor256.com/2018/03/27/how-to-test-thread-safety.html">Post</a>
* @todo #2375:90min. Implement a mechanism for "internal" and "external" classes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l3r8yJ, could you please provide more context? Imagine someone opening this issue in the future; they should be able to understand what to do at first glance:

  1. I still don't understand why we might need "internal" and "external" classes, or why we need to implement them.
  2. Why should "internal" classes be included in the catalog?
  3. Which specific catalog are you referring to?
  4. Why should "external" classes be placed in a different directory?
  5. Are "catalog" and "directory" the same entities in this context?

@volodya-lombrozo
Copy link
Member

volodya-lombrozo commented Apr 16, 2024

@volodya-lombrozo I made some clarifications based on your review. Also for better understanding of the term dirty classes, can you explain what means dirty in this context?

@l3r8yJ dirty refers to a type of class that was included in a jar during compilation but should not have been included. In other words, they are classes that were added to a jar by mistake.

Example:

Let's imagine a group of developers of the eo-strings library. Perhaps by mistake, they decided to create an uber-jar. So now, the eo-strings jar file contains not only Java atoms but also all dependencies (specifically, all the classes from these dependencies), including the eo-runtime library. Therefore, we call these classes dirty - they are here by mistake and shouldn't be.

Now, imagine you are trying to build another library that uses eo-strings and eo-runtime. When you compile your library, you will end up in a situation where you have two versions of eo-runtime:

your-eo-library-dependencies:
-> eo-strings -> eo-runtime:x
-> eo-runtime:y

This is a problem that is sometimes hard to identify. So we "clean" these dirty classes, which actually just hides the problem.

@l3r8yJ
Copy link
Contributor Author

l3r8yJ commented Apr 25, 2024

@volodya-lombrozo so, might be this issue can be resolved with some plugin, which will validate project on uber-jar? Plugin should be applied to library itself, wdyt? It should exclude such cases

@volodya-lombrozo
Copy link
Member

@l3r8yJ, I believe we should have protection on both sides, in our compiler and during the building of libraries. I'm afraid that creating a new plugin might be some sort of overengineering. But I like the idea of libraries validation.

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

Successfully merging this pull request may close these issues.

TranspileMojo.java:262-267: Add concurrency tests for the...
2 participants