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

Removed Semantic MediaWiki as a requirement #22

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

Conversation

yaronkoren
Copy link
Contributor

As noted in the comments for the README change, this file too needs to be changed; assuming that there's agreement that Semantic MediaWiki is not in fact a requirement. I also modified the extension description, and changed the URL to what I assume is the correct URL.

As noted in the comments for the README change, this file too needs to be changed; assuming that there's agreement that Semantic MediaWiki is not in fact a requirement. I also modified the extension description, and changed the URL to what I assume is the correct URL.
@mwjames
Copy link
Contributor

mwjames commented Feb 10, 2015

I'm in the process of some additional re-factoring with changes that will most likely require SMW therefore I can't +1.

"mediawiki/semantic-forms": "~2.8|~3.0"
},
"require-dev": {
"mediawiki/semantic-media-wiki": "@dev"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think there is some dependency on SMW at present, either in the production code or in the tests. Need to verify this is not the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests require SMW (already now -> 1.0.0).

@JeroenDeDauw
Copy link
Member

Change looks good, though needs verification.

@mwjames Let's please not add a dependency on SMW for the sake of it. That is not in the best interest of the users.

@mwjames
Copy link
Contributor

mwjames commented Feb 10, 2015

Let's please not add a dependency on SMW for the sake of it. That is not in the best interest of the users.

Sorry but I'm getting quite tired of this now, the code has been dead for 1.5 years without anyone taken notice and now for some reason it becomes a battle field whether it requires SMW or not is beyond me.

@JeroenDeDauw
Copy link
Member

Then why are you contesting this change? I also do not see why we'd need to create a fuzz about this.

@mwjames
Copy link
Contributor

mwjames commented Feb 10, 2015

Then why are you contesting this change? I also do not see why we'd need to create a fuzz about this.

First of all because it breaks current tests (which of course would have been noticed by #18) ergo we don't merge with failing tests.

@mwjames
Copy link
Contributor

mwjames commented Feb 10, 2015

The longer I look at the code the more I question whether SF is the right tool for what is being the sole purpose of this extension namely to "Allows adding additional fields to the user registration form, which get placed on the new users' user page, as well as stored semantically." but it doesn't mean I'm kicking the can by removing SF from the requirements list just because it seems to fit some agenda.

@yaronkoren
Copy link
Contributor Author

I certainly don't mean to cause drama, so I'm fine with this patch being dropped if it would lead to any complications. Though I still think it makes sense to merge in the change to the README file, since I was able to download the extension (via Git) and use it fine, with SMW not installed.

@mwjames
Copy link
Contributor

mwjames commented Feb 10, 2015

Though I still think it makes sense to merge in the change to the README file, since I was able to download the extension (via Git)

This should be true for 1.0.0 but starting with f3b0f16 we rely on the PSR-4 autoloader to invoke the classes.

@JeroenDeDauw
Copy link
Member

This should be true for 1.0.0 but starting with f3b0f16

Oh... I thought we had done that already before 1.0.

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