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

Space syntax importer #525

Merged
merged 12 commits into from
Mar 14, 2018
Merged

Space syntax importer #525

merged 12 commits into from
Mar 14, 2018

Conversation

thanosbnt
Copy link
Contributor

@thanosbnt thanosbnt commented Mar 13, 2018

Description

added space syntax Open Mapping importer

Checklist

  • Created new test(s) (if applicable)
  • Updated the README / documentation (if applicable)

@thanosbnt thanosbnt requested a review from arya-hemanshu March 13, 2018 13:49
@lorenaqendro lorenaqendro added this to the Release Candidate milestone Mar 14, 2018
@arya-hemanshu arya-hemanshu requested review from lorenaqendro and removed request for arya-hemanshu March 14, 2018 15:28
Copy link
Contributor

@lorenaqendro lorenaqendro left a comment

Choose a reason for hiding this comment

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

I believe this could be included in the code base. Please create a new issue for its refactoring with some problematic points we know already.

* The full dataset can be found at https://github.com/spacesyntax/OpenMapping
* Please see https://github.com/spacesyntax/OpenMapping for code of conduct and terms of use.
*
* How to use in a recipe:
Copy link
Contributor

Choose a reason for hiding this comment

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

Other info missing, subjectType, provider etc

Copy link
Contributor

Choose a reason for hiding this comment

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

done

if exists it updates else adds, but that increase overhead and compute time.
Using this method will only keep the old value and discard the new one, in case of
duplicate records.
Fix Me: Need to find a better way to address it
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add a FIXME tag here so we can get it easily from the IDE and add an issue for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

#530 is created

fixedValue.getId().getAttribute().getLabel(),
e.getMessage());
}
if ( saved % 2000 == 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a short comment of why 2000. It will be useful when rethinking this method as per FIXME.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

private int subjectCount, timedValueCount, fixedValueCount = 0;

private enum DatasourceId {
SpaceSyntaxOpenMapping(new DatasourceSpec(
Copy link
Contributor

Choose a reason for hiding this comment

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

could be shorter SSxOpenMap

@thanosbnt thanosbnt merged commit 16e615e into master Mar 14, 2018
@lorenaqendro lorenaqendro deleted the space_syntax_importer branch March 16, 2018 15:36
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