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

query current location & center map #12

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

Conversation

derhuerst
Copy link
Contributor

closes #2.

@derhuerst
Copy link
Contributor Author

bump. (:

@rastapasta
Copy link
Owner

aaa will soon be back online and on the code again - thanks for the bump :)

@derhuerst
Copy link
Contributor Author

bump.

3 similar comments
@derhuerst
Copy link
Contributor Author

bump.

@derhuerst
Copy link
Contributor Author

bump.

@derhuerst
Copy link
Contributor Author

bump.

@jaller94
Copy link
Collaborator

I think this is a cool feature, however, I have a couple of concerns.

  1. Mapscii gets used both locally and remotely via telnet. This feature would be solely available locally.
  2. It is platform dependent which ideally includes continuous integration tests on every targeted platform. For now the platform is simply NodeJS (and telnet).
    I do not have a MacOS device to test the feature now or in the future. It would be ideal to have an indicator whether it still works.
  3. There is a chance that Mapscii gets rejected from distribution channels, if binaries are included. In your project’s Readme you point to yet another repo, but without building steps. Is there a way to build the blob from source as part of the npm install process?
  4. The package is not published on NPM. The platform heavily invests in security audits and automated testing. It would be better to download a versioned package of the dependency from NPM.
  5. It is a convenience feature and as such might be implemented as an optional dependency. As the project already has optional dependencies, closely tied extensions could be a consideration.

Thanks so much, @derhuerst, for your contribution. I hope, we find a way to implement this without widening the technical scope of the project immensely.

@jaller94
Copy link
Collaborator

I am going to merge my CoffeeScript to JavaScript PR. Sorry for the merge conflict in advance.

@derhuerst
Copy link
Contributor Author

derhuerst commented Oct 17, 2018

  1. Mapscii gets used both locally and remotely via telnet. This feature would be solely available locally.

Correct. Should we detect wether Mapscii is running inside a telnet session?

  1. It is platform dependent which ideally includes continuous integration tests on every targeted platform. For now the platform is simply NodeJS (and telnet).

There are tests for macOS and Linux.

  1. There is a chance that Mapscii gets rejected from distribution channels, if binaries are included. In your project’s Readme you point to yet another repo, but without building steps.

There are building instructions in the Homebrew formula.

Is there a way to build the blob from source as part of the npm install process?

Yes, but it basically defeats the purpose of @derhuerst/location (being portable).

  1. The package is not published on NPM. The platform heavily invests in security audits and automated testing. It would be better to download a versioned package of the dependency from NPM.

There is @derhuerst/location. Back then, I didn't have in on npm yet.

  1. It is a convenience feature and as such might be implemented as an optional dependency. As the project already has optional dependencies, closely tied extensions could be a consideration.

How would that work?

Another possible way to get the current location is via a command line option: mapscii --location $(npx location-cli --comma).

@syui
Copy link

syui commented Jan 31, 2019

@derhuerst I think "query location" is a necessary function.
But, the command line option does not work.

# mac
$ git clone https://github.com/derhuerst/mapscii
$ cd mapscii
$ npm i
$ ./bin/mapscii.sh --location `curl -sL ipinfo.io/8.8.8.8/loc`

Do you have any thoughts about what the cause might be?

@derhuerst
Copy link
Contributor Author

Another possible way to get the current location is via a command line option: mapscii --location $(npx location-cli --comma).

But, the command line option does not work.
Do you have any thoughts about what the cause might be?

I either forgot to implement it, or to mention that it would be a cool addition. 😛

@derhuerst
Copy link
Contributor Author

I have adapted my PR to the current codebase, but I'd also be okay with only having the --location CLI option.

@@ -221,6 +223,9 @@ class Mapscii {
case 'c':
config.useBraille = !config.useBraille;
break;
case 'l':
Copy link

Choose a reason for hiding this comment

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

FWIW this would conflict with the changes made in fccacad :(

@YtvwlD
Copy link

YtvwlD commented Mar 7, 2020

What about using p for "position"?

@snipem
Copy link

snipem commented May 15, 2021

Would be nice to have this feature

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.

show current location
7 participants