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

review: feat: Add Environment#setFactory #1671

Closed
wants to merge 2 commits into from

Conversation

surli
Copy link
Collaborator

@surli surli commented Oct 30, 2017

Add a setFactory method to environment and set it when calling the main factory constructor with a given environment.
This allows to use the getFactory() of Environment without NPE.

@tdurieux
Copy link
Collaborator

Why the environment has a factory?

@surli
Copy link
Collaborator Author

surli commented Oct 30, 2017

Why the environment has a factory?

Because of the method getFactory I assume.
Anyway, I need it to access factory from DefaultJavaPrettyPrinter environment.

@tdurieux
Copy link
Collaborator

tdurieux commented Oct 30, 2017

My point is the getFactory in Environment makes no sense.

Currently, there is no usage of env.getFactory In DefaultJavaPrettyPrinter and only two usages of getFactory() in that class.

@surli
Copy link
Collaborator Author

surli commented Oct 30, 2017

My point is the getFactory in Environment makes no sense.

In that kind of situation: https://github.com/INRIA/spoon/pull/1672/files#diff-5f33bf5d1f38230de815e4c60a67a215

I consider it cleaner to call this.env.getFactory() than wildcardReference.getFactory().

@tdurieux
Copy link
Collaborator

tdurieux commented Oct 30, 2017

I do not agree, the Environment is only used for configuration and logging.
They should not be sate in the Environment.

The cycle reference Factory -> Environment; Environment -> Factory is really awful.

I looked where Environment.getFactory is used and it is only in the StandardEnvironment...
and getFactory can only return null.
I propose to remove getFactory from the Environment.

ps: we should update the JavaDoc of Environment which is completely outdated.

@monperrus
Copy link
Collaborator

I would agree with @tdurieux . The responsibility of Environment is to handle the configuration not the state.

A better solution would be to remove getFactory from Environment.

@surli
Copy link
Collaborator Author

surli commented Oct 30, 2017

The cycle reference Factory -> Environment; Environment -> Factory is really awful.

Agreed!
Just wanted to use it as it was already there, but I'm ok to remove it :)

@surli surli closed this Oct 30, 2017
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