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

Compatibility to BrianHenryIE/strauss #113

Closed
wants to merge 2 commits into from

Conversation

swarnat
Copy link

@swarnat swarnat commented Aug 31, 2024

fix: compatibility to BrianHenryIE/strauss

This is a very special modification for only some users, but which are currently impossible to use this helpfull library

BrianHenryIE/strauss library will prefix dependency classnames with a custom string to prevent dependency collision.

For example classname will be changed from Less_Visitor to ModA_Less_Visitor.
So multiple versions of same library can be used in ecosystems, where external modules can be installed from different developers.

The modified line generate function calls from class names, by removing the "namespace" part of class name.
When there is a custom prefix, then this line is not working.

The small modification check for custom prefix and append them to classname replacement.
Not a perfect solution. Maybe there is a better one.

fix: compatibility to BrianHenryIE/strauss
@wikimedia-helper-bot-for-github

👋 Hi @swarnat,

Thank you for contributing to Wikimedia! This Github repository is a read-only mirror.
The project uses Gerrit for code review.
Please submit your pull request to Gerrit, so your code changes can be reviewed.

If you are interested in sticking around within the Wikimedia community, please take a look at https://www.mediawiki.org/wiki/New_Developers
Thanks in advance!

fix: prevent prefixing the string to replace
@Krinkle
Copy link
Member

Krinkle commented Sep 1, 2024

I suggest to, instead of detecting a (generally non-existent) prefix, to focus only on extracting the suffix after "Less_Tree_". I prefer to 1) avoid encoding an assumption between Visitor and Tree, and 2) not support or maintain extra code for something we can't easily test for and would never happen outside of the code modification scenario, as such code is likely to seem as "unused code" and be break or be removed in the future by accident.

OptionalAnythingLess_Tree_Foo_Bar > Foo_Bar > FooBar

Changing the transform from prefix to suffix means:

  • the result is the same for the normal case
  • the logic remains specific to the Tree class, no assumption to Visitor
  • no extra code. Instead we replace the current code.same level of complexity instead of more.
  • Greater flexibility, as it allows any custom Tree or Visitor to work with the same code, not all or nothing.

@wikimedia-helper-bot-for-github

This comment was marked as duplicate.

@swarnat
Copy link
Author

swarnat commented Sep 1, 2024

I would agree with you, because to do a simple str_replace sounds a little bit unsecure in my case.
But yeah. Without your opinion and explanation I don't see a way to implement it, because there are classes like Less_Tree_Dimension and Less_Tree_Mixin_Definition.
So a simple split by underscore is not possible.

But yeah, because the "Less_Tree" must removed, I can also split by that string and take the last element.
This do not fulfill the "no extra code", but the other solution I see would be a substr combined by str_pos + strlen("Less_Tree")
I do not see any benefits different to the split.
My commit: 253969b

I already moved the diff to gerrit, because the bot don't like that here. :)
https://gerrit.wikimedia.org/r/c/mediawiki/libs/less.php/+/1069631

@Krinkle
Copy link
Member

Krinkle commented Sep 3, 2024

Thanks @swarnat. I left a suggestion there.

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

Successfully merging this pull request may close these issues.

2 participants