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

Easy refactor #36

Closed
wants to merge 10 commits into from
Closed

Easy refactor #36

wants to merge 10 commits into from

Conversation

pedroespindula
Copy link
Collaborator

No description provided.

Copy link
Owner

@brobsc brobsc left a comment

Choose a reason for hiding this comment

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

Da uma olhada ai.

O que tu nao entender/concordar me diz. Acho que tu nao precisa criar uma branch nova nao.

Se tiver tudo ok eu vou fazendo rebase aqui e ja junto direto. Mais rapido

import os


def create_exercise_file(name, path, exercise, cache):
Copy link
Owner

Choose a reason for hiding this comment

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

Aqui, em vez de ficar passando a cache o tempo todo tambem. Nao e melhor criar um objeto "student", que vem com o email, nome e, futuramente, matricula nao?

Ele ja viria criado la do tst_wrapper

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Certo, um dicionario?

Copy link
Owner

Choose a reason for hiding this comment

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

Exato

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

f.write(header(exercise, cache))


def formatted_unit(exercise):
Copy link
Owner

Choose a reason for hiding this comment

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

format_unit fica mais idiomatico, eu acho.

E por que nao ja adicionar "unidade" aqui tambem?

Copy link
Owner

Choose a reason for hiding this comment

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

No caso, pra t1, ele ja retornaria unidade01, e nao so 01

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Verdade, vou fazer isso.


# Get unit number from unit_raw
for num in unit_raw:
if num in '0123456789':
Copy link
Owner

Choose a reason for hiding this comment

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

Ja que tas refatorando, usa isdigit() aqui

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

return result.encode('utf-8')


def is_logged_in(response):
Copy link
Owner

Choose a reason for hiding this comment

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

Acho melhor o wrapper ficar com essa parte. A gente pede qualquer coisa a ele, e se nao tiver logado ele levanta uma exception, porque isso aqui ta muito intriseco ao tst, nao acha?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eu acho que essa função não ta sendo usada, posso remover?

Copy link
Owner

Choose a reason for hiding this comment

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

E da erro onde? Se não tiver logado...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ele faz a verificação dentro da própria função que precisa verificar, ele não chama a função, entendeu?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah beleza . Testasse se ele falha direto no watcher, quando vai tentar dar checkout de primeira?

Copy link
Collaborator Author

@pedroespindula pedroespindula Jul 1, 2018

Choose a reason for hiding this comment

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

Tentei, deu certo

path = os.path.expanduser('~/') + config['tst_root'] + 'unidade' + get_unit(ex) + '/'
else:
path = os.path.expanduser('~/') + config['tst_root']
path = os.path.expanduser('~/') + 'exercicios-tst/' + 'unidade' + easy_helper.formatted_unit(exercise) + '/'
Copy link
Owner

Choose a reason for hiding this comment

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

Isso aqui era uma gambiarra que eu tinha feito haha. Queres aproveitar pra usar os.path.join()?

Senao, depois eu faco.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Como assim?

Copy link
Owner

Choose a reason for hiding this comment

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

Isso aqui resolve aquela questão de ter ou não um “/“ no final, e corretamente formata o path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Acho melhor tu mudar isso depois se tu quiser.

# FIXME: Erasing directory to update it. Currently a workaround.
else:
print('''
def actualize_checkout(path, final_path, code):
Copy link
Owner

Choose a reason for hiding this comment

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

update_checkout

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

)
# Encode is needed because of accents on Programação and Matrícula
return result.encode('utf-8')
def rename_directory(path, exercise, code):
Copy link
Owner

Choose a reason for hiding this comment

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

Isso aqui mexe com diretorios, acho que e coisa do helper.

# Check if a file is empty
def is_zero_file(fpath):
return not (os.path.isfile(fpath) and os.path.getsize(fpath) > 0)
def cache_atualizer():
Copy link
Owner

Choose a reason for hiding this comment

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

update_cache()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

return result.encode('utf-8')
def rename_directory(path, exercise, code):
# Define final path
final_path = '{}{}/'.format(path, exercise['name'])
Copy link
Owner

Choose a reason for hiding this comment

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

Aqui tambem mexe com nome do path. Usa os.path.join()

Outra coisa. Acho bom tu separar isso aqui numa funcao, que recebe o exercicio, e retorna o path final adequado para ele. Pois vou usar la no organizer depois.



def main(code):
cache = cache_atualizer()
Copy link
Owner

Choose a reason for hiding this comment

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

Aqui a cache vai ser sempre atualizada quando for chamado o wrapper. Checa se ela ta vazia, se sim, update_cache(). Se nao, ela mesmo. Lembra daquele CACHE que tinha no clipboard.

Ai la no checkout (linha 36) ve se o exercicio ta presente. Se nao tiver, vai ser necessario atualizar a cache, pois pode ser um exercicio novo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ai voltaria a ter aquela variável global?

Copy link
Owner

Choose a reason for hiding this comment

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

No momento não vejo como resolver sem ser global nesse ponto. Senão vamos voltar àquele problema de perfomance (um request por checkout)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok


# If its not on cache, update current assignments
Copy link
Owner

Choose a reason for hiding this comment

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

Isso aqui seria um else, senao fica com codigo duplicado, como ta la embaixo.

label, code = ex['checkout_name'], ex['label']
# Get the specific exercise and return it
all_exercises = cache['assignments']
exercise = [e for e in all_exercises if e['checkout_name'] == code][0]
Copy link
Owner

Choose a reason for hiding this comment

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

Olha aqui ele duplicado


def rename_directory(path, exercise, code):
Copy link
Owner

Choose a reason for hiding this comment

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

Faltou mandar isso aqui pro easy_helper


def rename_directory(path, exercise, code):
# Define final path
final_path = '{}{}/'.format(path, exercise['name'])
Copy link
Owner

Choose a reason for hiding this comment

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

E colocar isso aqui final_path como uma funcao propria la

@pedroespindula pedroespindula deleted the easy-refactor branch July 1, 2018 18:40
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.

2 participants