Skip to content

Dependency level control #6

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

Merged
merged 5 commits into from
Jan 17, 2016
Merged

Dependency level control #6

merged 5 commits into from
Jan 17, 2016

Conversation

OneLastTry
Copy link

Hey,
I added an option to control the hierarchy level that is displayed on screen.
For some reason was falling with huge chain of classes.
Have a look if is interesting to add this
Cheers

@nikitaeverywhere
Copy link
Member

Hello! Very interesting commits, thank you! I will be able to test them till the end of this week - I have some priority work right now.

Some brief comments according to your commit:

  • For package.json metadata file we should keep semantic versioning so that fix to version 1.13.2 should be 1.13.3, but not 1.13.2.X.
  • What's the purpose of renaming global ClassExplorer to UClassExplorer?

@OneLastTry
Copy link
Author

Hey.

Ok, I will change the semantic. Also I will change the default value of this new parameter to null, so we keep the original behaviour untouched.

The global naming is more because I am planning to add this to an existing environment, just to avoid conflict.

I have some ideas to make this more suitable without changing codes, I'll try to work on these when I have some free time.

Cheers!

@nikitaeverywhere
Copy link
Member

The global naming is more because I am planning to add this to an existing environment, just to avoid conflict.

When I named this global I thought that noone will create global named as application is. The same I have in my WebTerminal project, the application global is named ^WebTerminal.

Anyway, the purpose is only not to conflict with system or default globals - because ^UClassExplorer can also be conflicted somewhere as well as ^ClassExplorer, ^WebTerminal, ^KJHFVaUfgKUFBLJSGDFJsf, etc.

If you have any conflicts, you can always install ClassExplorer into the new namespace, just create and import application to it, ClassExplorer won't loose any functionality and still be able to render classes from another namespace.

Looking forward to hear what do you think about the global names :)

Thank you for contributing!

@OneLastTry
Copy link
Author

Personally, I also think that this will never conflict. Anyways I was just being cautious, maybe too much =P
I will also revert this back.

BTW, very nice work! It is a nice tool.

@nikitaeverywhere
Copy link
Member

I have taken a look of how it works - nice!

But I found some things which I cannot understand - could you please answer the comments in your commits?

Also, as all the code indented with 4 spaces it should not be mixed with tab indents. But that is just a tip for future - when I merge commits, I will override tabs.

And you have changed the mechanism how settings are stored to allow not only to work with Boolean values - that's cool and seems that it works fine. I just want to clarify which exception catches this try and why there's no try block wrapped here, because the code in this two places looks pretty much the same.

Once all this things will be clarified, I'll merge the pull request. Thank you very much for contributing!

@OneLastTry
Copy link
Author

1 . $GET(level) can be "", and check turns to false;

Is just because this parameters is not being passed by all callers, I've changed to a better away tough.

2 . As I expect, setting the value of level to, for example, 3 will render the class, it's superclass, and the superclass of the superclass, all located in the same package in case of rendering the class (not the whole package render). But it seems that this check will not allow to display this supers. Check out SAMPLES namespace, class Cinema.Film. Rendering without level will normally display Cinema.Duration superclass, while even level=9 won't show the superclass. I don't think this is expected, what do you think?

I was trying to reduce as much as possible the view, but it seems not be working fine. This is removed.

That looks strange for me. Even more strange in the head commit. The Type of the property/parameter/xData/query/method/etc meant to be always as Caché tells us, and client side of UMLExplorer make post-processing based on this types.
Here I see that you modify the Type for some cases after correct type is set. And $Char(32) may demolish any post-processing on the client, because in case you have PackageName.ClassName\n instead of PackageName.ClassName, ClassExplorer won't recognize any dependencies between the classes.
Could you please explain for what cases do we need to modify type in such way? I really have no idea.
UPD
What an ignore flag do in the head commit? I think it definitely must be set for both if (..classExists(package _ "." _ p.Type)) and elseif (..classExists(..extendClassFromType(p.Type))) condition branches, doesn't it? But I cannot catch the logic of this.

The ignore and this whole part, was an attempt to not display inheritances outside the main class (the selected one). But this is also tricky. Adding $Char(32) at the end, was ending the link as I expected. But the behaviour is different when checking more the one level. So, this is also removed.

@nikitaeverywhere
Copy link
Member

OK, great, the functionality you have added is useful - thank you very much!

I was trying to reduce as much as possible the view, but it seems not be working fine. This is removed.

I will fix the inheritance dependency level (I assume currLevel should be incremented each next fall into inheritance) after merge.

Thank you for your contribution!

nikitaeverywhere pushed a commit that referenced this pull request Jan 17, 2016
Dependency level control
@nikitaeverywhere nikitaeverywhere merged commit deb3baf into intersystems-community:master Jan 17, 2016
@nikitaeverywhere
Copy link
Member

Released!

@OneLastTry
Copy link
Author

I appreciate that, my pleasure to contribute!

Thx for all the support.

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.

3 participants