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

Add package tags as a semantic entity #929

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

iglosiggio
Copy link

This way we can quickly reference and navigate to a given package tag.

Made during the pharo sprint with @matijakljajic

@iglosiggio iglosiggio force-pushed the browse-to-package-tag branch from 2df4eb0 to 1e1784d Compare January 31, 2025 16:38
@Ducasse
Copy link
Contributor

Ducasse commented Feb 2, 2025

Thanks I will have a look.
Would be good to have tests and getXXX is not that idiomatic of Pharo.
I will have a look.

@Ducasse
Copy link
Contributor

Ducasse commented Feb 2, 2025

I know that the existing code is using this getX stuff.

@iglosiggio
Copy link
Author

Yes, we continued the code following what was already there. Now it's probably time to improve it.

OTOH there's no test on this PR and we are adding a new feature. That's not right :P

My rewrite would be:

  • Remove the object mutation from the getX methods
  • Add a field containing the list of supported "entity providers" (an entity provider is something that respondsTo #value: and expects the token list as argument)
  • Make the initial value of this list something like: { [ :tokens | self maybeGetClass ]. [ :tokens | self maybeGetPackage ]. [ :tokens | self maybeGetTag ] } (if we put a bit more work we can migrate the whole computeEntity method to a series of calls like that)
  • Then the implementation of computeEntity is just entity := providers detect: [ :provider | provider value: tokens ].

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.

2 participants