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

[BUG]: Is the linter, Check Angular Services Index Required #19462

Open
jnvtnguyen opened this issue Jan 13, 2024 · 7 comments
Open

[BUG]: Is the linter, Check Angular Services Index Required #19462

jnvtnguyen opened this issue Jan 13, 2024 · 7 comments
Labels
Impact: Low -- DO NOT WORK ON THIS YET Postponing for now, since it doesn't affect users much. needs debugging

Comments

@jnvtnguyen
Copy link
Contributor

jnvtnguyen commented Jan 13, 2024

Describe the bug

When I create an angular injectable, say ModeratorAuthGuard, the linter will complain when I don't add it to the angular-services.index.ts file here:

def _check_angular_services_index(self) -> concurrent_task_utils.TaskResult:
"""Finds all @Injectable classes and makes sure that they are added to
Oppia root and Angular Services Index.
Returns:
TaskResult. TaskResult having all the messages returned by the
lint checks.
"""
name = 'Angular Services Index file'
error_messages: List[str] = []
injectable_pattern = '%s%s' % (
'Injectable\\({\\n*\\s*providedIn: \'root\'\\n*}\\)\\n',
'export class ([A-Za-z0-9]*)')
angular_services_index_path = (
'./core/templates/services/angular-services.index.ts')
angular_services_index = self.file_cache.read(
angular_services_index_path)
error_messages = []
failed = False
for file_path in self.ts_files:
file_content = self.file_cache.read(file_path)
class_names = re.findall(injectable_pattern, file_content)
for class_name in class_names:
if class_name in INJECTABLES_TO_IGNORE:
continue
import_statement_regex = 'import {[\\s*\\w+,]*%s' % class_name
if not re.search(
import_statement_regex, angular_services_index):
error_message = (
'Please import %s to Angular Services Index file in %s'
'from %s'
% (class_name, angular_services_index_path, file_path))
error_messages.append(error_message)
failed = True
service_name_type_pair_regex = (
'\\[\'%s\',\\n*\\s*%s\\]' % (class_name, class_name))
service_name_type_pair = (
'[\'%s\', %s]' % (class_name, class_name))
if not re.search(
service_name_type_pair_regex, angular_services_index):
error_message = (
'Please add the pair %s to the angularServices in %s'
% (service_name_type_pair, angular_services_index_path)
)
error_messages.append(error_message)
failed = True
return concurrent_task_utils.TaskResult(
name, failed, error_messages, error_messages)

URL of the page where the issue is observed.

N/A

Steps To Reproduce

N/A

Expected Behavior

Is this lint check required?

Screenshots/Videos

No response

What device are you using?

Desktop

Operating System

Windows

What browsers are you seeing the problem on?

No response

Browser version

No response

Additional context

No response

Tips for developers

Before tackling the bug, please use git bisect (see https://git-scm.com/docs/git-bisect) to investigate which PR caused it. If the issue occurred before commit c7db0fe, the most effective approach is to utilize the Python setup for conducting a git bisect (You only need to go back as far as commit https://github.com/oppia/oppia/commits/9a334e9). If you need to perform a git bisect for any commit beyond that commit, the Docker setup will be suitable. If you find the PR, leave a comment on this issue pointing to it, or just say that it happened before commit 9a334e9 if you could reproduce it there. This will help us fix the issue by reverting the faulty PR.

Also, if this is your first issue, please make sure to follow https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue and https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#setting-things-up before claiming it. Thanks!

@jnvtnguyen jnvtnguyen added bug Label to indicate an issue is a regression triage needed labels Jan 13, 2024
@retrogtx
Copy link

retrogtx commented Jan 14, 2024

Hi @seanlip,
I would like to work on the issue, I think I can solve it.

It seems like you're asking for a way to automatically add a newly created Angular Injectable to the angular-services.index.ts file to avoid linter complaints.

Files changed: js_ts_linter.py
The following script script can automate this:

image

add_to_index(service_name: str, index_file_path: str = "path/to/angular-services.index.ts") This function is used to add a new Angular service to the angular-services.index.ts file. It takes two arguments: service_name which is the name of the service to be added, and index_file_path which is the path to the angular-services.index.ts file. The function opens the index file, reads all lines, and inserts import and export statements for the new service at the first empty line it finds. After modifying the lines, it writes them back to the file.

@DubeySandeep
Copy link
Member

@retrogtx We are not expecting to add any automation here, instead we are trying to understand why we need this check and whether it is still needed, if not we can remove it!

@srijanreddy98 Do you know why we need this check and when can this be removed?

@DubeySandeep DubeySandeep added needs debugging Impact: Low -- DO NOT WORK ON THIS YET Postponing for now, since it doesn't affect users much. and removed triage needed bug Label to indicate an issue is a regression labels Jan 14, 2024
@retrogtx
Copy link

my bad, noob behaviour from my side :-/

@DubeySandeep
Copy link
Member

@retrogtx If you are looking for an issue to work on, I would suggest trying #19435 ! Let me know if you have any other questions!

@retrogtx
Copy link

thanks will look into it!

@Nadiya-Shaikh
Copy link

Out of issue question: May I know for running the frontend test cases what dependencies I will have to install? I'm new here please guide me!!

@srijanreddy98
Copy link
Member

It should be safe to remove the check but it is not yet safe to remove the angular-services index file. They have a unit test presence with importAllAngularServices and a runtime presence in Oppia root (where it is only present to import a predefined set of services.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: Low -- DO NOT WORK ON THIS YET Postponing for now, since it doesn't affect users much. needs debugging
Projects
Development

No branches or pull requests

5 participants