fix(Oxidized::String): implement Oxidized::String using refinements a… #2750
Conversation
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
from
e3d5c1f
to
c1943dd
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #2750 +/- ##
===========================================
- Coverage 66.40% 19.94% -46.46%
===========================================
Files 30 13 -17
Lines 1500 692 -808
===========================================
- Hits 996 138 -858
- Misses 504 554 +50
... and 22 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
3 times, most recently
from
f119ac8
to
e36a210
Compare
|
I tried running this to verify the fix for Issue #2749 , but it crashed out at the first attempt to scrape a config from a JunOS device. Now , it's not finding a |
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
from
e36a210
to
bf9ba92
Compare
|
Thank you @neilschelly , I'll keep working on it :) |
|
I can confirm the error even for comware: Unfortunately I'm not good enough in ruby to fix this. :( I have written tests that are able to access |
|
Got it working @neilschelly. Could you try with the latest commit e75fd27 |
|
Now, I'm getting the |
|
@neilschelly apparently I forgot I changed two things at once and only commited once. So I do need to add them to the Model itself too. please try again. I fixed Junos for you too. If that works I need to do it to every model.. |
|
This is working for me now, no errors and the Comware plugin is working correctly. Thanks so much for your work on this so quickly! I was not going to come up with a solution this clean. |
|
Thank you, me neither, it was @ytti who pointed me in the right direction. I'll have to fix all other models and then release 0.29.1 tonight. |
lib/oxidized/model/outputs.rb
Outdated
| module Oxidized | ||
| class Model | ||
| using Oxidized::StringRefinements |
There was a problem hiding this comment.
We don't have to mention Oxidized, as we are also under this namespace, so the search path will hit us. Idiomatically we don't mention it, when we don't have to.
I personally still would prefer Oxidized::String to be module included in Refinements module which refines Strings by including the Oxidized::String module, as in the comware ticket. But this is more matter of taste than anything I can justify.
The justification being, maybe we add refinements to other classes, and then we don't have to change anything, as we already included the Refinements module, which contains not just String but now something else.
And including module instead of listing it directly, because we can have them in separate files cleanly.
Like maybe a refinements.rb including string and any futureType.
And require 'refinements'. using Refinements.
There was a problem hiding this comment.
I like it, and I'll have a go at it, dunno if I can make it though :|
There was a problem hiding this comment.
I personally still would prefer Oxidized::String to be module included in Refinements module which refines Strings by including the Oxidized::String module, as in the comware ticket. But this is more matter of taste than anything I can justify.
I don't think that's possible. Or I don't know how.
I can create a Refinements Module and put the code there, so we only need that. But I don't think it's possible to "include" Oxidized::String in a Refinements module.
Also I do not believe we can use using Refinements. but we'd have to do using Refinements::StringRefinements.
There was a problem hiding this comment.
we could do:
module Refinements
# Use the 'refine' keyword to define refinements for the String class
refine String do
[...]
end
refine Array do
[...]
end
endclass Foobar
using Refinements
[...]and that would allow us to refine String and Array at the same time.
There was a problem hiding this comment.
@ytti , so i tried to do come as close as possible to your wish. Please let me know if that's acceptable now :)
I think we really want to avoid needing to require these in models themselves, and have the refinement come from the parent. Adding boilerplate code in models itself would be undesirable. I can't quickly find a way to do this to the model subclasses with refinements dynamically, only with monkey patching. So either I'm missing something, or we need boilerplate or we need to just monkey patch String. It looks like for pre/post we will inherit the refinements, as instance_eval is exception to the rules that stop refinements from being inherited. So there might be a way to make the refinements work in models without boilerplate. Would need to commit more time to figure this out if refinements without boilerplate is doable. |
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
3 times, most recently
from
97e321f
to
4eee60e
Compare
|
here's a summary of the change:
|
I wish we could avoid this. I don't like that we need boiler plate for model makers. Almost feels like outright monkey-patching String is better even with the risks involved, of refinements cannot be inherited or dynamically inserted (I tried, and couldn't find a way). One option that I think might work, for example in old: def cmd(string, &block)
....
endnew: def cmd(string, &block)
block.binding.eval("using Refinements")
...
end&block is the code user supplies, here we're getting the binding for the block then evaluating code inside that binding, this should mean as if user had written ``ùsing Refinements``` in the block itself. |
|
I understand your concerns, but using refinements already pushed my ruby knowledge way over the limit. I want to help the project but I'll mostly focus on pipelines and tooling. Hence I suggest let's merge this and fix 0.29.0 by releasing 0.29.1, and then either you or someone else finds another solution. I check and clean the git commits, so when someone wants to rewrite it, it is going to be a simple git revert of just one commit. Hope that's okay :) |
|
Got feedback from @ytti , will merge and release new patch :) |
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
4 times, most recently
from
3940d4d
to
6324a54
Compare
…lib/refinements.rb` Closes: ytti#2749
aschaber1
force-pushed
the
2749-comware-issues-with-cut_both-in-0290
branch
from
6324a54
to
e47d719
Compare
|
just wanted to mention that i discovered this breaking change recently because i receive this error: and that's where I found out that something changed, as you mention I have to add |
…nd adding them to comware as PoC
Closes: #2749
Pre-Request Checklist
rubocop --auto-correct)rake test)Description