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

Improve editor command line arguments #34

Merged
merged 1 commit into from Nov 28, 2017

Conversation

Projects
None yet
2 participants
@xsawyerx
Contributor

xsawyerx commented Nov 13, 2017

Instead of assuming the command line arguments are fit for opening in a particular line and column, we now translate based on the command line.

The only proper way would be to include information on how each editor needs its arguments.

If the editor is unknown, we simply use the editor parameter with the filename, which is the only likely thing to at least work:

$ENV{EDITOR} file.pl

If we were to try to also introduce the line and column numbers, it is more likely to fail than succeed because each editor parses command line differently.

A problem that still needs to be addressed is the usage of $App::Critique::CONFIG{CRITIQUE_EDITOR} as the same value as $App::Critique::COFNIG{EDITOR} as the same value as $ENV{EDITOR}.

These are different and should not be mixed together. It would likely require some more subtle logic in Critique.pm, hopefully removing the BEGIN {} block at the process. :)

This small bit includes a test. It is not numbered because I don't see the point of numbering tests.

Improve editor command line arguments:
Instead of assuming the command line arguments are fit for opening in a
particular line and column, we now translate based on the command line.

The only proper way would be to include information on how each editor
needs its arguments.

If the editor is unknown, we simply use the editor parameter with the
filename, which is the only likely thing to at least work:

    $ENV{EDITOR} file.pl

If we were to try to also introduce the line and column numbers, it is
more likely to fail than succeed because each editor parses command line
differently.

A problem that still needs to be addressed is the usage of
$App::Critique::CONFIG{CRITIQUE_EDITOR} as the same value as
$App::Critique::COFNIG{EDITOR} as the same value as $ENV{EDITOR}.

These are different and should not be mixed together. It would likely
require some more subtle logic in Critique.pm, hopefully removing the
BEGIN {} block at the process. :)

This small bit includes a test. It is not numbered because I don't see
the point of numbering tests.
@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Nov 13, 2017

Contributor

Small fix made and commit amended. It's good now. Tested in production.

Contributor

xsawyerx commented Nov 13, 2017

Small fix made and commit amended. It's good now. Tested in production.

@stevan

This comment has been minimized.

Show comment
Hide comment
@stevan

stevan Nov 15, 2017

Owner

You said to not merge this, so I am closing it. For the record, I merged #36 because it pretty much solves the problem. That said, we should make a CPAN module to do this, it should be a solved problem.

Owner

stevan commented Nov 15, 2017

You said to not merge this, so I am closing it. For the record, I merged #36 because it pretty much solves the problem. That said, we should make a CPAN module to do this, it should be a solved problem.

@stevan stevan closed this Nov 15, 2017

@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Nov 16, 2017

Contributor

Err... I had said to pause on merging and then I fixed the problem. That was the message saying "it's good now" and "tested in production". :)

Contributor

xsawyerx commented Nov 16, 2017

Err... I had said to pause on merging and then I fixed the problem. That was the message saying "it's good now" and "tested in production". :)

@stevan

This comment has been minimized.

Show comment
Hide comment
@stevan

stevan Nov 18, 2017

Owner

Whoops, misunderstood the cross channel communication then, re-opening and will take a look at merging it

Owner

stevan commented Nov 18, 2017

Whoops, misunderstood the cross channel communication then, re-opening and will take a look at merging it

@stevan stevan reopened this Nov 18, 2017

@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Nov 18, 2017

Contributor

I think the mistake is that we had two implementations. @roman-alexeev and I should have coordinated.

Mine splits the logic to a separate file and has two hashes: One for translation of each editor (allowing to easily add more) and one for resolving aliases of editors (likesubl).

The other implementation does the translation in the configuration resolution step.

The other implementation fails for sublime-text (because it doesn't support subl as an alias) but that could be fixed with a smarter regexp.

They both work so it doesn't matter which you take.

Contributor

xsawyerx commented Nov 18, 2017

I think the mistake is that we had two implementations. @roman-alexeev and I should have coordinated.

Mine splits the logic to a separate file and has two hashes: One for translation of each editor (allowing to easily add more) and one for resolving aliases of editors (likesubl).

The other implementation does the translation in the configuration resolution step.

The other implementation fails for sublime-text (because it doesn't support subl as an alias) but that could be fixed with a smarter regexp.

They both work so it doesn't matter which you take.

@stevan stevan merged commit b923630 into stevan:master Nov 28, 2017

1 check passed

code-analysis/kritika Kritika analysis finished
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment