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

Patch debug details #363

Closed
wants to merge 1 commit into from

Conversation

AnonMiraj
Copy link

i tried to not refactor the code too much for this small change.
i have tested it and it works as intended

@AnonMiraj AnonMiraj requested a review from gnikit as a code owner March 17, 2024 07:16
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.52%. Comparing base (3386073) to head (91b810a).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #363   +/-   ##
=======================================
  Coverage   87.52%   87.52%           
=======================================
  Files          35       35           
  Lines        4760     4763    +3     
=======================================
+ Hits         4166     4169    +3     
  Misses        594      594           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@gnikit gnikit left a comment

Choose a reason for hiding this comment

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

Good first crack at this but this does not really fix the issue.
The useful thing is communicating the final (i.e. combined CLI + config) initialisation settings of the LanguageServer. This only restates what the CLI and config inputs are, which the user can just retrieve from looking at their fortls settings. (Also, you shouldn't be using sys.argv to retrieve the CLI options).

The solution I had in mind was rearchitecting parts of the constructor and initialization methods. Generally, care must be taken since certain variables like root_path are only available from the RPC request of the serve_initialization and could not be delegated to the constructor of the class.

@AnonMiraj
Copy link
Author

thanks for the advice.
i now understand what needs to be done
i will try implementing them accordingly

@AnonMiraj AnonMiraj marked this pull request as draft March 20, 2024 02:10
@AnonMiraj
Copy link
Author

The solution I had in mind was rearchitecting parts of the constructor and initialization methods. Generally, care must be taken since certain variables like root_path are only available from the RPC request of the serve_initialization and could not be delegated to the constructor of the class.

quick question
since all the options are are already stored the class
cant we just log these values directly or write them to a json file so it is cleaner ?

{
    "root_path": "/home/nero/Documents/library/Programming/test/test",
    "workspace": "{}",
    "obj_tree": "{}",
    "link_version": "0",
    "_version": "0.1.dev1064+g3386073",
    "config": ".fortlsrc",
    "nthreads": "4",
    "notify_init": "False",
    "incremental_sync": "False",
    "sort_keywords": "False",
    "disable_autoupdate": "False",
    "preserve_keyword_order": "False",
    "debug_log": "True",
    "source_dirs": "{'/path/to/source'}",
    "incl_suffixes": "set()",
    "excl_suffixes": "set()",
    "excl_paths": "{'/path/to/exclude'}",
    "autocomplete_no_prefix": "False",
    "autocomplete_no_snippets": "False",
    "autocomplete_name_only": "False",
    "lowercase_intrinsics": "False",
    "use_signature_help": "False",
    "variable_hover": "False",
    "hover_signature": "False",
    "hover_language": "fortran90",
}

would this be correct or am i missing something?

@AnonMiraj AnonMiraj reopened this Mar 20, 2024
@AnonMiraj AnonMiraj requested a review from gnikit March 20, 2024 23:49
@AnonMiraj AnonMiraj marked this pull request as ready for review March 20, 2024 23:51
@gnikit
Copy link
Member

gnikit commented Mar 21, 2024

cant we just log these values directly or write them to a json file so it is cleaner ...
would this be correct or am i missing something?

Not quite, because that would be duplicating the CLI options inside the language server.
The idea of passing settings and then creating class attributes at runtime is to not have to explicitly write the options of the CLI in multiple places.

@gnikit
Copy link
Member

gnikit commented Apr 1, 2024

After our discussion over at Fortran-lang I will be closing this PR now.

@gnikit gnikit closed this Apr 1, 2024
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.

None yet

2 participants